[PATCH] D73934: [mlir] Add support for basic location translation to LLVM.

[Alex Zinenko via Phabricator via llvm-commits]

ftynse accepted this revision.
ftynse added a comment.
This revision is now accepted and ready to land.

Thanks River, this is very useful!

I'm not an expert on LLVM debuginfo though.



================
Comment at: mlir/lib/Target/LLVMIR/DebugTranslation.cpp:28
+DebugTranslation::DebugTranslation(Operation *module, llvm::Module &llvmModule)
+    : builder(llvmModule), llvmCtx(llvmModule.getContext()) {
+  // If the module has no location information, there is nothing to do.
----------------
I'd explicitly initialize compileUnit to nullptr since it is later used as a signaling state for not doing anything.


================
Comment at: mlir/lib/Target/LLVMIR/DebugTranslation.cpp:30
+  // If the module has no location information, there is nothing to do.
+  if (!module->walk(locationInteruptWalker).wasInterrupted())
+    return;
----------------
Nit: I was confused by the name of the operation into believing that it would interrupt in _absence_ of location info, but it's the inverse. The code is correct though.


================
Comment at: mlir/lib/Target/LLVMIR/DebugTranslation.cpp:33
+
+  // TODO(riverriddle) This isn't actually correct. This is fine for now as
+  // we only emit line-table information and not any Types or variables.
----------------
Nit: please specify what exactly isn't correct (e.g., using C as language)


================
Comment at: mlir/lib/Target/LLVMIR/DebugTranslation.cpp:38
+      builder.createFile(llvmModule.getModuleIdentifier(), "/"), "mlir",
+      /*isOptimized=*/true, "", 0);
+
----------------
Nit: could we have parameter names in comments for `""` and `0` as well?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73934/new/

https://reviews.llvm.org/D73934