[PATCH] D94981: [LiveDebugVariables] Add cache for SkipPHIsLabelsAndDebug to prevent iterating the same set of PHI/LABEL/Debug instructions repeatedly.

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:1311-1312
+      // Note the iterator kept in BBSkipInstsMap is either the iterator
+      // before the one returned by SkipPHIsLabelsAndDebug last time,
+      // or MBB->begin(). We need to keep one iterator before the
+      // iterator returned by SkipPHIsLabelsAndDebug because there may be
----------------
Is this comment incorrect? It sounds like from your other comments that MBB->begin() is never in the map?


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:1327
+      auto I = MBB->SkipPHIsLabelsAndDebug(BeginIt);
+      if (I != MBB->begin())
+        BBSkipInstsMap[MBB] = std::prev(I);
----------------
I guess this could be `I != BeginIt` - could save updates when the search doesn't find a new spot? & might be marginally cheaper comparison than calling begin() again?


================
Comment at: llvm/lib/CodeGen/LiveDebugVariables.cpp:1309-1315
+      if (MachineInstr *BI = BBSkipInstsMap[MBB])
+        BeginIt = std::next(MachineBasicBlock::iterator(BI));
+      else
+        BeginIt = MBB->begin();
+      MachineBasicBlock::iterator I = MBB->SkipPHIsLabelsAndDebug(BeginIt);
+      if (I != MBB->begin())
+        BBSkipInstsMap[MBB] = &*std::prev(I);
----------------
wmi wrote:
> dblaikie wrote:
> > Could you store iterators in the map instead of pointers? Would save having to do the `&*` dance on the way in and the `MachineBasicBlock::iterator` dance on the way out?
> > Also, it'd be good to avoid the extra BBSkipInstsMap lookup, if possible. Would this work?
> > ```
> > auto& I = BBSkipInstsMap.insert(MBB, MBB->begin()).first->second;
> > I = MBB->SkipPHIsLabelsAndDebug(I);
> > return I;
> > ```
> > Could you store iterators in the map instead of pointers? Would save having to do the &* dance on the way in and the MachineBasicBlock::iterator dance on the way out?
> 
> Good idea. MachineBasicBlock is an ilist so iterator should be valid after new instruction is inserted if ilist has the same property as std::list. 
> 
> > Also, it'd be good to avoid the extra BBSkipInstsMap lookup, if possible. Would this work?
> > auto& I = BBSkipInstsMap.insert(MBB, MBB->begin()).first->second;
> > I = MBB->SkipPHIsLabelsAndDebug(I);
> > return I;
> 
> That may not work. If we insert MBB->begin() into the map, after the last call of SkipPHIsLabelsAndDebug, there may be new non-phis/non-labels/non-debuginstrs inserted at the beginning of MBB and the old begin iterator saved in the map will become stale. When we call SkipPHIsLabelsAndDebug next time, it will skip those non-phis/labels/debug instructions which is not welcomed.
> 
> Note for the case that the return value of SkipPHIsLabelsAndDebug is not MBB->begin(), we will save std::prev of the returned iterator because the newly inserted instructions after last call of SkipPHIsLabelsAndDebug will be searched next time.
Hmm, not quite following.

>  after the last call of SkipPHIsLabelsAndDebug, there may be new non-phis/non-labels/non-debuginstrs inserted at the beginning of MBB and the old begin iterator saved in the map will become stale. When we call SkipPHIsLabelsAndDebug next time, it will skip those non-phis/labels/debug instructions which is not welcomed.

Wouldn't that problem exist also... oh, I see. That's why you go back by 1.

Hmm. I guess if the map contained `Optional<iterator>` values it might look like this:

```
auto &O = BBSkipInstsMap[MBB];
auto B = MBB->Begin();
auto I = MBB->SkipPHIsLabelsAndDebug(O ? std::next(*O) : B);
if (I != B)
  O.emplace(std::prev(I));
```

Simplifies the code somewhat, avoids the double-lookup, but adds a bunch of memory overhead - making the values larger, and storing lots of None values where the current code avoids the entry entirely. So probably not worth it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94981/new/

https://reviews.llvm.org/D94981