[PATCH] D11933: Extending debug ranges

[Adrian Prantl via llvm-commits]

aprantl added a comment.

I just manually verified that this indeed works as advertised on a couple of old bugreports with debug info + ASAN. Totally awesome!
Couple more inline comments and I think the only major thing left to do here is to agree on a name for the new pass :-)

It worries me a bit that none of the test cases exhibit the case with multiple predecessors that the join() function is implementing (nested ifs without else, switch stmts...). It would be good to add one.


================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:182
@@ +181,3 @@
+  return (Var == V.Var) && (isDescribedByReg(*MI) == isDescribedByReg(*V.MI)) &&
+         (areOffsetsEqual(*MI, *V.MI));
+}
----------------
What if MI is, e.g., constant 1 and V.MI is constant 2? Looks like the function would still return true because isDescribedByReg will return 0 of both. We should also compare the kind (const/reg/...) here.

Regression/Unit test that catches these corner cases?

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:284
@@ +283,3 @@
+  // joined.
+  for (auto p = MBB.pred_begin(), pe = MBB.pred_end(); p != pe; ++p) {
+    auto OL = OutLocs.find(*p);
----------------
This can now be converted to a range-based for.

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:339
@@ +338,3 @@
+      VarLoc V(ILT.Var, MI);
+      ILL.push_back(std::move(V));
+    }
----------------
Why is it necessary to push it into InLocs here? Wouldn't transfer pick it up automatically from the DBG_VALUE that we just inserted?

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:386
@@ +385,3 @@
+        OLChanged = false;
+        for (MachineBasicBlock::const_succ_iterator si = MBB->succ_begin(),
+                                                    se = MBB->succ_end();
----------------
This can now be converted to a range-based for.


http://reviews.llvm.org/D11933