[PATCH] D154080: [DebugInfo][RemoveDIs] Add conversion utilities between dbg.value form and DPValue new-debug-info form
Orlando Cazalet-Hyams via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 4 03:08:06 PDT 2023
Orlando added a comment.
LGTM with some minor questions
================
Comment at: llvm/include/llvm/IR/BasicBlock.h:93
+
+ /// Set the value of the IsNewDbgInfoFormat flag.
+ void setIsNewDbgInfoFormat(bool NewFlag);
----------------
I think this comment could be slightly misleading - the function appears to set the flag //and // change the block to the desired debug info format.
================
Comment at: llvm/include/llvm/IR/Function.h:101-103
+ /// Is this function using intrinsics to record the position of debugging
+ /// information, or non-intrinsic records?
+ bool IsNewDbgInfoFormat;
----------------
I think it's worth copying the comment you have in `BasicBlock.h` here (or redirecting to it) - that one clearly identifies what true and false indicate.
================
Comment at: llvm/include/llvm/IR/Module.h:218
+ /// information, or non-intrinsic records?
+ bool IsNewDbgInfoFormat;
+
----------------
See my comment in `Function.h`
================
Comment at: llvm/lib/IR/BasicBlock.cpp:31
+cl::opt<bool> UseNewDbgInfoFormat("experimental-debuginfo-iterators",
+ cl::desc("Enable communicating debuginfo positions through iterators, eliminating intrinsics"),
+ cl::init(false));
----------------
clang-format
================
Comment at: llvm/lib/IR/BasicBlock.cpp:57
+ assert(!I.DbgMarker && "DbgMarker already set on old-format instrs?");
+ if (DbgValueInst *DVI = dyn_cast<DbgValueInst>(&I)) {
+ // Convert this dbg.value to a DPValue.
----------------
What about other debug intrinsics? What's the plan for those?
================
Comment at: llvm/lib/IR/BasicBlock.cpp:124
+
+ // Only validate if we have a debug program.
+ if (!IsNewDbgInfoFormat)
----------------
================
Comment at: llvm/lib/IR/BasicBlock.cpp:189
+ IsNewDbgInfoFormat = false;
if (NewParent)
----------------
Any reason not to put in the member initialiser list?
================
Comment at: llvm/lib/IR/BasicBlock.cpp:236-238
+ for (auto &Inst : *this) {
+ if (!Inst.DbgMarker)
+ continue;
----------------
This pattern is used in a few places. Could be worth using a `filter_iterator` (`llvm::make_filter_range` etc), similar to what `instructionsWithoutDebug()` does, that skips instructions without debug markers? YMMV.
e.g.
`for (auto &Inst : instructionsWithDebugMarkers())`
================
Comment at: llvm/lib/IR/Verifier.cpp:2896
+ // errors and return true if there's a problem.
+ bool RetVal = BB.validateDbgValues(false, true);
+ Check(!RetVal, "Invalid configuration of new-debug-info data found");
----------------
I think the verifier has a `raw_ostream` member `OS`; better to pass that in than always printing to stderr in `validateDbgValues`?
================
Comment at: llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp:1777-1778
assert(BB != PreEntryBlock && "Don't copy the preetntry block");
BasicBlock *NewBB = CloneBasicBlock(BB, VMap, ".nonchr", &F);
+ NewBB->setIsNewDbgInfoFormat(F.IsNewDbgInfoFormat);
NewBlocks.push_back(NewBB);
----------------
Shouldn't `CloneBasicBlock` be responsible for this?
================
Comment at: llvm/unittests/IR/DebugInfoTest.cpp:779-782
FirstInst->DbgMarker = new DPMarker();
FirstInst->DbgMarker->MarkedInstr = FirstInst;
RetInst->DbgMarker = new DPMarker();
RetInst->DbgMarker->MarkedInstr = RetInst;
----------------
As discussed in the previous patch can we please now use `createMarker` and friends in this function?
================
Comment at: llvm/unittests/IR/DebugInfoTest.cpp:976
+ EXPECT_EQ(DVI2->getVariableLocationOp(0), FirstInst);
+
+ UseNewDbgInfoFormat = false;
----------------
Possibly worth confirming the other dbg.value fields are what we expect to see?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154080/new/
https://reviews.llvm.org/D154080
More information about the llvm-commits
mailing list