[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-1092
+    auto PseudoValue = Operand->getPseudoValue();
+    if (PseudoValue->kind() == PseudoSourceValue::FixedStack) {
+      auto FS = cast<FixedStackPseudoSourceValue>(PseudoValue);
----------------
dmgreen wrote:
> getPseudoValue can return nullptr on failure, which needs to be checked.
> The type should be `const PseudoSourceValue *`, not auto (the type isn't obvious from the function call).
> Then you might be able use `if (const auto *FS = dyn_cast<FixedStackPseudoSourceValue>(PseudoValue))`
> 
> Or you may be able to use `if (const auto *FS = dyn_cast_or_null<FixedStackPseudoSourceValue>(PseudoValue))` to get the same effect?
The `dyn_cast<...>` option looks good to me. 👍 


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1106
+      return I->getOperand(1).getReg() == ARM::SP &&
+             GetFrameIndex(I->memoperands().front(), FI);
+    }
----------------
dmgreen wrote:
> This will have to check that it has at least one memoperand.
Ah I thought that would be guaranteed by the instruction being a V[STR|LDR]WU32. I'll add a check.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1121
+  // live-out (which sp never is) to know what blocks to look in
+  unsigned FI;
+  GetFrameIndex(MI->memoperands().front(), FI);
----------------
dmgreen wrote:
> Frame indexes look like they are usually int's.
> 
> Does GetFrameIndex return the value by parameter reference because FrameIndexes can be negative? So we can't return -1 for not found?
I chose for it to be taken as reference so that a boolean could be returned to indicate success, but if it can just return the frame index or -1 in case of error then that would be great. From looking through the codebase (this is a great use-case for more function documentation) it looks like frame indexes will be positive so I think returning -1 in case of failure is a good option.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1124
+
+  SmallVector<MachineBasicBlock *, 1> Frontier;
+  ML->getExitBlocks(Frontier);
----------------
dmgreen wrote:
> I think you can remove the ", 1", and let it choose a default small size. Same below.
`SmallPtrSet` requires an initial size but I can remove it for `SmallVector`.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1139
+      // can ignore the block
+      if (I.getOpcode() == ARM::MVE_VSTRWU32)
+        break;
----------------
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.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1148
+    for (auto Succ : BB->successors()) {
+      if (!Visited.contains(Succ))
+        Frontier.push_back(Succ);
----------------
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.


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

https://reviews.llvm.org/D105443