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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

The searching feels gnarly, like it can end up searching a lot of instructions. I don't know of a better idea though, and it should be rare that it needs to search.



================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1087
 
-bool LowOverheadLoop::ValidateMVEInst(MachineInstr* MI) {
+bool ValidateMVEStore(MachineInstr *MI, MachineLoop *ML) {
+
----------------
This function can be made static.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1090-1092
+    auto PseudoValue = Operand->getPseudoValue();
+    if (PseudoValue->kind() == PseudoSourceValue::FixedStack) {
+      auto FS = cast<FixedStackPseudoSourceValue>(PseudoValue);
----------------
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?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1106
+      return I->getOperand(1).getReg() == ARM::SP &&
+             GetFrameIndex(I->memoperands().front(), FI);
+    }
----------------
This will have to check that it has at least one memoperand.


================
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);
----------------
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?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1122
+  unsigned FI;
+  GetFrameIndex(MI->memoperands().front(), FI);
+
----------------
Check for isSpillSlotObjectIndex(FI) too?


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


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1139
+      // can ignore the block
+      if (I.getOpcode() == ARM::MVE_VSTRWU32)
+        break;
----------------
If we find another stack store, does that mean we can stop the search?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1148
+    for (auto Succ : BB->successors()) {
+      if (!Visited.contains(Succ))
+        Frontier.push_back(Succ);
----------------
Should this check that Succ is not already in Frontier too?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1143
   // will be fine, but ensure that all store operations are predicated.
-  if (MI->mayStore())
-    return IsUse;
+  // An unpredicated vector register spill is allowed if all of the uses of the
+  // stack slot are within the loop
----------------
samtebbs wrote:
> dmgreen wrote:
> > Is it worth splitting this out into a new function? To keep this stack logic separate in case it needs to grow even more sophisticated in the future.
> Good idea. Do you think a lambda or proper function would be best?
As a function looks good. It's nice and self-contained.


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

https://reviews.llvm.org/D105443