[PATCH] D11933: Extending debug ranges

[Adrian Prantl via llvm-commits]

aprantl added a comment.

Thanks of the update, this is starting to look really good. I have a couple of comments inline that are mostly about readability. I'll play with the patch and look at the correctness next. Did you make any compile time measurements with the new algorithm? For example, by compiling clang?


================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:67
@@ +66,3 @@
+  typedef std::pair<const DILocalVariable *, const DILocation *>
+      InlinedVariable;
+
----------------
I found InlinedVariable to be misleading because the Inlined-At is optional. What about just calling it a DebugVariable and promoting it to a struct?


```
/// A potentially inlined instance of a variable.
struct DebugVar {
  const DILocalVariable &Var;
  const DILocation *InlinedAt;
}
```

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:79
@@ +78,3 @@
+  typedef std::list<VarLoc> VarLocList;
+  typedef std::map<const MachineBasicBlock *, VarLocList> VarLocInMBB;
+
----------------
This should probably be a (Small)DenseSet (=hash map) instead of a std::map.

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:87
@@ +86,3 @@
+  VarLocInMBB OutLocs;   // Ranges that exist beyond bb.
+  VarLocInMBB InLocs;    // Ranges that are incoming after joining.
+
----------------
Instead of having them as members that are cleared for each function, it would be better to make them local variables that are explicitly passed as arguments. It's less error-prone and makes the code easier to read because there is no global state.

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:199
@@ +198,3 @@
+  // End all previous ranges of Var.
+  auto ORi = OpenRanges.begin(), ORe = OpenRanges.end();
+  while (ORi != ORe) {
----------------
This entire loop could be replaced by a use of std::remove_if with the condition in a lambda. I believe it would be more readable this way.

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:222
@@ +221,3 @@
+    for (MCRegAliasIterator RAI(MO.getReg(), TRI, true); RAI.isValid(); ++RAI) {
+      auto ORi = OpenRanges.begin(), ORe = OpenRanges.end();
+      while (ORi != ORe) {
----------------
Again, this can be expressed more compactly with std::remove_if.

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:256
@@ +255,3 @@
+    DEBUG(dbgs() << "Add to OutLocs: "; OR.MI->dump(););
+    if (std::find_if(VLL.begin(), VLL.end(), [&](const VarLoc &V) {
+          return (V.Var == OR.Var) &&
----------------
Since this comparison is duplicated three times why not implement an operator== for VarLoc?

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:305
@@ +304,3 @@
+    auto ILT = InLocsT.begin(), ILTe = InLocsT.end();
+    while (ILT != ILTe) {
+      if (std::find_if(VLL.begin(), VLL.end(), [&](const VarLoc &V) {
----------------
With the operator== (see line 256) this pattern can probably also be collapsed to a std::remove_if.

================
Comment at: test/DebugInfo/X86/sret.ll:7
@@ -8,1 +6,3 @@
+; CHECK: DW_AT_GNU_dwo_id [DW_FORM_data8] ([[ADDR:0x[0-9a-z]*]])
+; CHECK: DW_AT_GNU_dwo_id [DW_FORM_data8] ([[ADDR]])
 
----------------
I believe, we actually want the dwo_id to be hardcoded in this one testcase, because it forces us to think why the hash changed, when it changed.



http://reviews.llvm.org/D11933