[PATCH] D105443: [ARM][LowOverheadLoops] Make some stack spills valid for tail predication

[Sam Tebbs via Phabricator via llvm-commits]

samtebbs added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1090
+  auto GetFrameIndex = [](MachineMemOperand *Operand) {
+    auto PseudoValue = Operand->getPseudoValue();
+    if (PseudoValue->kind() == PseudoSourceValue::FixedStack) {
----------------
dmgreen wrote:
> const PseudoSourceValue* PseudoValue =
> It still needs to check it's not null too. The store could have a non-pseudo MMO, for which this would return nullptr.
Cheers, I'll add that.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1139
+      // can ignore the block
+      if (I.getOpcode() == ARM::MVE_VSTRWU32)
+        break;
----------------
dmgreen wrote:
> samtebbs wrote:
> > dmgreen wrote:
> > > If we find another stack store, does that mean we can stop the search?
> > If we have found another store to the same stack slot then we stop the search as any further loads in the same BB won't invalidate the store we're validating, since they are dependent on that second store instead.
> Sure. I was thinking in that case it wouldn't have to add any Successors, so we could potentially save from looking through a lot of other BB's.
Ah yes that is a very good point! I'll make it account for that.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1148
+    for (auto Succ : BB->successors()) {
+      if (!Visited.contains(Succ))
+        Frontier.push_back(Succ);
----------------
dmgreen wrote:
> samtebbs wrote:
> > dmgreen wrote:
> > > Should this check that Succ is not already in Frontier too?
> > Yeah that can't hurt, however I can't find a nice `contains` function in `SmallVector`. I wish container classes had the same interface.
> There is an is_contained(Frontier, Succ) in ADT, that is a wrapper around std::find.
Sweet, thanks


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

https://reviews.llvm.org/D105443