[PATCH] D69945: [ARM][MVE] Tail predication conversion

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:46
+    MachineInstr *VCTP = nullptr;
+    SmallVector<MachineInstr*, 4> VPTMIs;
+    bool Revert = false;
----------------
Nit, perhaps: `VPTMIs` -> `VPTUsers`?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:48
+    bool Revert = false;
+    bool TailPredicate = false;
+    bool CannotTailPredicate = false;
----------------
Looks like this `TailPredicate` is a boolean for "FoundVPT", so can we just use `VCTP`? Or perphaps at least change the name because it might be easy to confuse it with `CannotTailPredicate`?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:98
+    // Is it safe to define LR with DLS/WLS?
+    // LR can defined if it is the operand to start, because it's the same
+    // value, or if it's going to be equivalent to the operand to Start.
----------------
nit: can defined -> can be defined / is defined


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:116
+        dbgs() << "ARM Loops: Not a low-overhead loop.\n";
+      else if (!(Start && Dec && End))
+        dbgs() << "ARM Loops: Failed to find all loop components.\n";
----------------
nit:   `else if (!FoundAllComponents())`


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:299
+    LLVM_DEBUG(dbgs() << "ARM Loops: LoopEnd is not targetting header.\n");
+    Revert = true;
+  }
----------------
nit: perhaps just `return` here?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:307
+    LLVM_DEBUG(dbgs() << "ARM Loops: LE offset is out-of-range\n");
+    Revert = true;
+  }
----------------
and here


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:320
+  if (!InsertPt) {
+    LLVM_DEBUG(dbgs() << "ARM Loops: Unable to find safe insertion point.\n");
+    Revert = true;
----------------
because the only thing we do is printing this, which doesn't add much to the debug messages already emitted.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:426
+          LoLoop.ScanForVPR(&MI);
+        LoLoop.CheckTPValidity(&MI);
       }
----------------
If we cannot tail-predicate, does that mean we keep checking TPValidity here? I think this is place where the usage of `CannotTailPredicate` and `TailPredicate` can be a bit confusing.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:552
+  MachineBasicBlock *MBB = InsertPt->getParent();
+  bool IsDo = Start->getOpcode() == ARM::t2DoLoopStart ? true : false;
+  unsigned Opc = 0;
----------------
Nit, just: `bool IsDo = Start->getOpcode() == ARM::t2DoLoopStart;`


================
Comment at: llvm/test/CodeGen/Thumb2/LowOverheadLoops/vector-arith-codegen.ll:392
+; Function Attrs: nofree norecurse nounwind
+define dso_local arm_aapcs_vfpcc void @vector_mul_vector_u8(i8* noalias nocapture %a, i8* noalias nocapture readonly %b, i8* noalias nocapture readonly %c, i32 %N) local_unnamed_addr #0 {
+; CHECK-LABEL: vector_mul_vector_u8:
----------------
If I'm not mistaken, function `@vector_mul_vector_u8` and `@vector_mul_vector_s8` above are exactly the same?


================
Comment at: llvm/test/CodeGen/Thumb2/LowOverheadLoops/vector-arith-codegen.ll:519
+
+define dso_local arm_aapcs_vfpcc void @vector_mul_vector_u16(i16* noalias nocapture %a, i16* noalias nocapture readonly %b, i16* noalias nocapture readonly %c, i32 %N) {
+; CHECK-LABEL: vector_mul_vector_u16:
----------------
Same with this and the function above?


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

https://reviews.llvm.org/D69945