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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
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,
----------------
david-arm wrote:
> 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. :)
Correct, the tripcount could overflow but then so would the up-rounded tripcount. When the VL is a power-of-2 then both values would overflow to '0'  and because it tests for inequality, the loop would break when both values are '0' so it just works without the runtime check. I'll update the comment!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1551
+  bool foldTailByMasking() const {
+    return getTailFoldingStyle() != TailFoldingStyle::None;
+  }
----------------
david-arm wrote:
> 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?
Fair point, the meaning of `FoldTailByMasking` is derived from `LoopVectorizationLegality::prepareToFoldTailByMasking` which checks whether the loop //can// use tail folding, so perhaps we can rename it to `LoopCanFoldTailByMasking` ?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9201
   addCanonicalIVRecipes(*Plan, Legal->getWidestInductionType(), DebugLoc(),
-                        true, CM.useActiveLaneMaskForControlFlow());
   return Plan;
----------------
david-arm wrote:
> 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. :)
Having this property based on the tail folding style seemed to be right thing to do and none of the tests break, so I suspect this was an omission in the previous implementation. Or it is never tested, but it seems odd to do something different in the two code paths to this function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142887