[PATCH] D11933: Extending debug ranges

[Adrian Prantl via llvm-commits]

aprantl added a comment.

Thanks, more comments inline!

Side note, the clang coding standards recommend to write every comment in the code as complete sentences, with a "." at the end.


================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:81
@@ +80,3 @@
+
+  VarLocList OpenRanges;   // Ranges that are open until end of bb
+  VarLocList OutgoingLocs; // Ranges that exist beyond bb
----------------
OpenRanges should probably be a local variable in the while (!BBWorklist.empty()) loop in ExtendRanges().

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:300
@@ +299,3 @@
+  for (auto OLo = OutgoingLocs.begin(), OLoe = OutgoingLocs.end();
+       OLo != OLoe; OLo++) { // outer loop
+    for (auto OLi = OutgoingLocs.begin(), OLie = OutgoingLocs.end();
----------------
This loop is quadratic //and// only works for two predecessors. Off the top of my head, what about, e.g, having a SmallDenseMap that stores a separate IncomingLocs for each MBB?

IncomingLocs could be a sorted SmallVector or another SmallDenseMap of 
```
struct IncomingLoc {
  DIVariable* Var;
  MachineInstr Loc;
};
```
A nullptr for Loc (or a missing entry) means that this value has been determined to be unknown.
Additionally each MBB gets a flag that indicates whether it has been visited or not.

At the end of each MBB, go through the list of successors and immediately join() the OutgoingLocs with the IncomingLocs of the successor. In the join function every input wins over the IncomingLocs of a not-yet-visited MBB, after the first visit the MBB is marked as visited. Likewise, a nullptr (or whatever we use to indicate the kill value) also wins against any other value.


================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:360
@@ +359,3 @@
+      DEBUG(print("After propagating joined MBB", dbgs()));
+
+      if (OLChanged) {
----------------
Ok. See my other comment about generalizing the algorithm.

================
Comment at: lib/CodeGen/ExtendDebugRangeLocation.cpp:362
@@ +361,3 @@
+      if (OLChanged) {
+        OLChanged = false;
+        for (MachineBasicBlock::const_succ_iterator si = MBB->succ_begin(),
----------------
Typically in LLVM transfer functions return bool and this would be written as:

```
bool Changed = false;
Changed |= transfer(MI)
```


http://reviews.llvm.org/D11933