[PATCH] D73934: [mlir] Add support for basic location translation to LLVM.
Alex Zinenko via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Feb 4 14:41:57 PST 2020
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
More information about the llvm-commits
mailing list