[PATCH] D11933: Extend debug ranges and provide multiple location support for debug variables

[Adrian Prantl via llvm-commits]

aprantl added a comment.

I didn't have a chance to actually play with the code so far (and the patch didn't apply cleanly), but here are some very high level comments and questions:

- Why did you choose to insert DBG_VALUEs into the machine function instead of implementing it directly inside DbgValueHistoryCalculator?
- Having it as a separate pass could actually be good for testability — did you look into using the brand new MIR serialization format for writing MIR->MIR testcases?
- There should also be a test case with an aggregate variable that is split across multiple DW_OP_pieces to make sure they are handled correctly.
- Did you run any benchmarks with the new pass? Does it cause any measurable slowdown?

The MultipleLocations handling should probably be split out into a separate patch and reviewed separately.

thanks,
adrian


================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.cpp:135
@@ -98,1 +134,3 @@
+  if (std::find(VarSet.begin(), VarSet.end(), Var) != VarSet.end())
+    return; // Ignore if already found
   VarSet.push_back(Var);
----------------
Comment should be above the return.

================
Comment at: lib/CodeGen/AsmPrinter/DbgValueHistoryCalculator.h:41
@@ -40,1 +40,3 @@
 private:
+  // The VarInstrRange map will have multiple open ranges because of the
+  // PreserveDbgValLoc flag. This means startInstrRange, endInstrRange and other
----------------
Just a general remark: Please follow
http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments
and use /// doxygen comments without \brief where possible.

================
Comment at: lib/CodeGen/DebugValueFixup.cpp:1
@@ +1,2 @@
+//===-- DebugValueFixup.cpp - Fixup Debug Value MIs -----------------------===//
+//
----------------
Fixup is not a really descriptive name. Really it inserts additional DEBUG_VALUES to extend the live ranges of debug locations. ExtendDebugLocationLiveRanges, DebugValueLiveRangeExtender, ...? 

================
Comment at: lib/CodeGen/DebugValueFixup.cpp:15
@@ +14,3 @@
+//    built in DbgValueHistoryCalculator.
+// b. Fixup multiple locations problem.
+// c. Add missing DBG_VALUE instructions, if possible (TODO).
----------------
Please elaborate.

================
Comment at: lib/CodeGen/DebugValueFixup.cpp:19
@@ +18,3 @@
+//
+// Initially this logic was supposed to be implemented inside
+// DbgValueHistoryCalculator code. But there are situations where the
----------------
Should be rephrased as "This is a separate pass from DbgValueHistoryCalculator because....".


http://reviews.llvm.org/D11933