[PATCH] D125301: [LoopVectorize] Add option to use active lane mask for loop control flow

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:162
 
+enum class ActiveLaneMask { None, Predicate, PredicateAndControlFlow };
+
----------------
Please say if I'm being picky but to me `ActiveLaneMask` is a thing, as in, it's the active lane mask.  Here I think you're asking the target to choose the style of predication. So something like the following reads better to me:
```
enum class PredicationStyle { None, Data, DataAndControlFlow };
```


================
Comment at: llvm/lib/Target/AArch64/AArch64TargetTransformInfo.h:336-337
+  ActiveLaneMask emitGetActiveLaneMask() const {
+    if (ST->hasSVE())
+      return ActiveLaneMask::PredicateAndControlFlow;
+    return ActiveLaneMask::None;
----------------
Will we do the correct thing if the vectoriser chooses fixed length vectorisation?  Not sure what "correct" really means here other than not crash or regress performance? I just want to ensure we've considered the possibility.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8727
+  }
+  EB->appendRecipe(BranchBack);
 }
----------------
Given the new block is already `EB->appendRecipe`ing several instructions I don't see any value in  breaking out the last instruction like this.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9171-9172
+  bool UseLaneMaskForControlFlow =
+      CM.foldTailByMasking() &&
+      CM.TTI.emitGetActiveLaneMask() == ActiveLaneMask::PredicateAndControlFlow;
   addCanonicalIVRecipes(*Plan, Legal->getWidestInductionType(), DebugLoc(),
----------------
This idiom occurs in a few places.  Is it worth adding something like `CM.useActiveLaneMaskForControlFlow()`.  I think it makes sense for this to imply tail folding by mask as why else would you be doing it.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:824
+
+      // Exit the loop if there are no lanes active. Testing the first lane is
+      // sufficient for this.
----------------
no active lanes?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1567
+  for (unsigned Part = 0, UF = State.UF; Part < UF; ++Part) {
+    Instruction *StartMask;
+
----------------
Why is this here? and not where `StartMask` is assigned a couple of lines below.


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

https://reviews.llvm.org/D125301