[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: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
----------------
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?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1147
+    if (MI->getOpcode() != ARM::MVE_VSTRWU32 ||
+        MI->getOperand(1).getReg() != ARM::SP)
+      return IsUse;
----------------
dmgreen wrote:
> 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.
Good spot, I'll have a look into that. Regarding other ways to find instructions that use a specific stack slot, I haven't been able to find a better way either.


================
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
----------------
dmgreen wrote:
> 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()) {
----------------
dmgreen wrote:
> 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