[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]

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