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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Thanks. This looks good, minus some mostly Nitpicks.



================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1090
+  auto GetFrameIndex = [](MachineMemOperand *Operand) {
+    auto PseudoValue = Operand->getPseudoValue();
+    if (PseudoValue->kind() == PseudoSourceValue::FixedStack) {
----------------
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.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1104
+      return I->getOperand(1).getReg() == ARM::SP &&
+             I->memoperands().size() >= 1 &&
+             GetFrameIndex(I->memoperands().front()) >= 0;
----------------
 == 1 is probably fine. I wouldn't expect it to be higher.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1131
+  SmallPtrSet<MachineBasicBlock *, 2> Visited{MI->getParent()};
+  unsigned int Idx = 0;
+  while (Idx < Frontier.size()) {
----------------
unsigned int -> unsigned, is more common in llvm.


================
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);
----------------
samtebbs wrote:
> 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.
I think they may be able to go negative, but a negative argument means a parameter passed via the stack (I think). So treating it the same as an invalid value sounds like it should work fine.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1124
+
+  SmallVector<MachineBasicBlock *, 1> Frontier;
+  ML->getExitBlocks(Frontier);
----------------
samtebbs wrote:
> 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`.
Oh of course, yeah. I hadn't realized it didn't have the same interface.

SmallPtrSet sounds good, but you may want to increase the default to 4. It won't make a large difference, but 4 pointers are not going to hurt anyone.


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


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


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

https://reviews.llvm.org/D105443