[PATCH] D71837: [ARM][MVE] Tail Predicate IsSafeToRemove

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMBaseInstrInfo.h:660
+  return Opc == ARM::SUBri ||
+         Opc == ARM::tSUBi3 || Opc == ARM::tSUBi8 ||
+         Opc == ARM::t2SUBri || Opc == ARM::t2SUBri12 || Opc == ARM::t2SUBSri;
----------------
What about `tSUBSi3`, do we need to consider that too?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:376
+                           SmallPtrSetImpl<MachineInstr*> &Ignore) {
+  auto getLocalUses = [&RDA](MachineInstr *MI,
+                             SmallPtrSetImpl<MachineInstr*> &Uses) {
----------------
style: personally I think `getLocalUses` could be a local static helper function. It is a lambda, but pretty big, and only invoked here. I.e. it isn't e.g. used as an object that is passed to an algorithm. 


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:388
+      // and if so, then check the users before MI. If we find that the value
+      // if live-out in any other block then we can't remove it.
+      if (auto *LiveOut = RDA->getLocalLiveOutMIDef(MI->getParent(),
----------------
typo: if -> is


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:518
 
-  LLVM_DEBUG(dbgs() << "ARM Loops: Will use tail predication.\n");
+  // Check that the value change of the element count is what we expect and
+  // that the predication will be equivalent. For this we need:
----------------
I wanted to comment if you could be more specific what you mean with `value change` and `what we expect` here, but reading a bit further clarified that, but perhaps....


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:522
+  // and we can also allow register copies within the chain too.
+  auto IsValidSub = [](MachineInstr *MI, unsigned VecWidth) {
+    unsigned ImmOpIdx = 0;
----------------
...renaming `VecWidth` to `ExpectedVecWidth` or something along those lines help a bit.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:524
+    unsigned ImmOpIdx = 0;
+    switch (MI->getOpcode()) {
+    default:
----------------
Is this also a little helper function that could live in ARMBaseInstrInfo as e.g. `getSubImmOpIndex`?


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

https://reviews.llvm.org/D71837