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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
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
----------------
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.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1147
+    if (MI->getOpcode() != ARM::MVE_VSTRWU32 ||
+        MI->getOperand(1).getReg() != ARM::SP)
+      return IsUse;
----------------
I think we may need to distinguish between a load that accesses SP and a stack spill slot. A SP address could be some other (potentially escaping) stack object. I think that info is usually available through the MMO, maybe via FrameInfo::isSpillSlotObjectIndex.

I was looking around for a way to get all the MachineInstr's that access a specific stack slot, but couldn't fine one better than searching through a lot of instructions.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1150-1152
+    // Find uses of the same stack slot. ReachingDefAnalysis doesn't work for sp
+    // as it relies on registers being live-out (which sp never is) to know what
+    // blocks to look in
----------------
I think this comment would be better if we explain what we do do, not why we don't use RDA (or maybe both). Explaining that we are searching all blocks after the loop for another access to the same stack slot.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1157
+    SmallPtrSet<MachineBasicBlock *, 2> Visited{MI->getParent()};
+    unsigned int i = 0;
+    while (i < Frontier.size()) {
----------------
Use `I` or `It` or `Idx` or something?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105443