[PATCH] D142887: [LoopVectorize][TTI] NFCI: Clarify enum for the tail folding style.

[David Sherwood via Phabricator via llvm-commits]

david-arm added a reviewer: paulwalker-arm.
david-arm added a subscriber: paulwalker-arm.
david-arm added a comment.

This patch looks good and it saves us having two orthogonal states! I just had a few minor comments ...



================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:165
 
-enum class PredicationStyle { None, Data, DataAndControlFlow };
+enum class TailFoldingStyle {
+  /// Don't use tail folding
----------------
Given @paulwalker-arm was involved in the naming scheme for the original enum (https://reviews.llvm.org/D125301) it's probably worth adding him as a reviewer on the patch.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:172
+  /// the tripcount rounded up by VL, which when the VL is a power-of-2 will
+  /// never cause any overflow.
+  Data,
----------------
I'm not sure this statement is quite true. I think the tripcount will overflow, but will overflow to 0 for powers of 2. The induction variable will also overflow to 0 and so the comparison used to branch back is safe, i.e.

  %iv.next = add i64 %iv, %vl
  %keep_going = icmp ne %iv.next, %rounded.up.trip.count
  br i1 %keep_going, %vector.body, %middle.block

I am not sure if this was by design, or that it's just pure luck that this works. :)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1551
+  bool foldTailByMasking() const {
+    return getTailFoldingStyle() != TailFoldingStyle::None;
+  }
----------------
Hi @sdesmalen, it seems a little odd to have an accessor method `foldTailByMasking` that doesn't return the underlying variable of the same name `FoldTailByMasking`. It's more an observation really - I just wondered if the function should be named differently to the variable?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1557
   bool useActiveLaneMaskForControlFlow() const {
-    return FoldTailByMasking &&
-           TTI.emitGetActiveLaneMask() == PredicationStyle::DataAndControlFlow;
+    return FoldTailByMasking && TTI.getPreferredTailFoldingStyle() ==
+                                    TailFoldingStyle::DataAndControlFlow;
----------------
Can this be more simply written as

  return getTailFoldingStyle() == TailFoldingStyle::DataAndControlFlow

?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9201
   addCanonicalIVRecipes(*Plan, Legal->getWidestInductionType(), DebugLoc(),
-                        true, CM.useActiveLaneMaskForControlFlow());
   return Plan;
----------------
Previously this was setting HasNUW=true when tail-folding without the active lane mask, whereas now in addCanonicalIVRecipes we only set to true if not tail-folding at all. I'm not sure if this matters? Certainly no tests seem to break. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142887