[PATCH] D65884: [ARM] MVE Tail Predication

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: lib/Target/ARM/Thumb2TailPredication.cpp:95
+
+static bool IsSetup(Instruction &I) {
+  auto *Call = dyn_cast<IntrinsicInst>(&I);
----------------
nit: perhaps slightly more informative function name, something with "LoopIterations" in it?


================
Comment at: lib/Target/ARM/Thumb2TailPredication.cpp:134
+  if (!ST->hasMVEIntegerOps() || !ST->hasLOB()) {
+    LLVM_DEBUG(dbgs() << "TP: Wrong Subtarget.\n");
+    return false;
----------------
"Wrong Subtarget"  -> perhaps better/clearer to say that Subtarget does not support TP.



================
Comment at: lib/Target/ARM/Thumb2TailPredication.cpp:138
+
+  LLVM_DEBUG(dbgs() << "TP: Running Tail Predication on " << *L << "\n");
+  BasicBlock *Preheader = L->getLoopPreheader();
----------------
Perhaps better to move this message to the next block as it might not be really running at this point (if there's no preheader)?


================
Comment at: lib/Target/ARM/Thumb2TailPredication.cpp:145
+  IntrinsicInst *Setup = nullptr;
+  for (auto &I : *Preheader) {
+    if (IsSetup(I)) {
----------------
Bit of a nit, but perhaps setting 'Setup' can be a function, so that we don't have the `if (!Setup)` check below twice, but simply a return when we found it.


================
Comment at: lib/Target/ARM/Thumb2TailPredication.cpp:152
+
+  // The test.set iteration could live in the pre- preheader.
+  if (!Setup) {
----------------
nit: extra whitespace in "pre- preheader"


================
Comment at: lib/Target/ARM/Thumb2TailPredication.cpp:187
+
+bool TPCandidate::isTailPredicate(Value *V, Value *NumElements) {
+  // Look for the following:
----------------
I am not sure, but I'm wondering if this function is misnamed? I think I would expect `isTailPredicate` to work on a Loop, not a Instruction/Value.


================
Comment at: lib/Target/ARM/Thumb2TailPredication.cpp:204
+  // %induction = add <4 x i32> %broadcast.splat, <i32 0, i32 1, i32 2, i32 3>
+  // %pred = icmp ule <4 x i32> %induction, %broadcast.splat11
+  using namespace PatternMatch;
----------------
I need to read this function again, but it isn't clear to me yet why this is then a tail-predicated loop and not some other vectorized loop. 


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

https://reviews.llvm.org/D65884