[PATCH] D98849: [LV] Compute ranges for plans up front (NFC).

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Thanks @fhahn for moving this forward!
Have mostly minor nits.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:208
-  const TargetLibraryInfo *TLI;
-
   /// Target Transform Info.
----------------
This cleanup is independent?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:301
 
+  /// Compute a list of vector factor ranges so that the decisions for each
+  /// instruction matches across all vector factors in the range.
----------------
vector factor >> vectorization factor (x2)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:304
+  SmallVector<VFRange, 4>
+  computeRanges(ElementCount MinVF, ElementCount MaxVF,
+                SmallPtrSetImpl<Instruction *> &DeadInstructions);
----------------
computeRanges >> computeVFRanges


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1596
 
+  /// Returns true if \p CI should be widened for \p VF.
+  bool shouldWidenCall(CallInst *CI, ElementCount VF) {
----------------
Ideally these `bool CM.should*(inst, VF)` methods would be const, but that seems to require various other methods to be const as well?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1633
+
+  /// Returns true if instruction \p I will be should be for \p VF.
+  bool shouldScalarize(Instruction *I, ElementCount VF) {
----------------
will be should be >> should be scalarized


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8451
-             ID == Intrinsic::experimental_noalias_scope_decl))
-    return nullptr;
-
----------------
Note that folding the above VF-agnostic early-exit into the lambda below may cause some overhead - no need to scan the entire range to realize it will not be clamped. But as your measurements confirm this is insignificant.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8473
       },
       Range);
 
----------------
Should one-liner 'willWiden()' lambda's be folded inline into getDecisionAndClampRange() as in IsPredicated above?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8493
   // scalarization is profitable or it is predicated.
-  auto WillScalarize = [this, I](ElementCount VF) -> bool {
-    return CM.isScalarAfterVectorization(I, VF) ||
-           CM.isProfitableToScalarize(I, VF) ||
-           CM.isScalarWithPredication(I, VF);
+  auto ShouldScalarize = [this, I](ElementCount VF) -> bool {
+    return CM.shouldScalarize(I, VF);
----------------
How about 'ShouldScalarize' >> 'willWiden' returning !CM.shouldScalarize(), to be more consistent with the above?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8685
+  Ranges.emplace_back(MinVF, MaxVF.getWithIncrement(1));
+
+  auto SplitRange = [&Ranges](unsigned Idx, ElementCount VF) {
----------------
// Split the [Start, End) Range at Idx into [Start, VF*2) and [VF*2, End)

by clamping the End of the existing Range and introducing a new range.
Can Range.Start continue to be const?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8714
+
+  for (BasicBlock *BB : make_range(DFS.beginRPO(), DFS.endRPO())) {
+    for (Instruction &I : BB->instructionsWithoutDebug()) {
----------------
Better scan basic-blocks in regular DFS order, avoid RPO.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8739
+          bool Split = false;
+          bool Widened = false;
+          if (CM.isOptimizableIVTruncate(&I, VF)) {
----------------
Widened >> Widen

Better first set Widen = CM.*(Instr, VF) and then set Split = (Widen == CM.*(Instr, VF*2)) throughout?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8742
+            Split |= CM.isOptimizableIVTruncate(&I, VF) !=
+                     CM.isOptimizableIVTruncate(&I, VF * 2);
+            Widened = true;
----------------
Use `Instr ` or `&I` consistently?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8750
+            Split |=
+                CM.shouldWidenMemory(&I, VF) != CM.shouldWidenMemory(&I, VF);
+            Widened |= CM.shouldWidenMemory(&I, VF);
----------------
second `VF` should be `VF * 2`
(calls for a test?)

Alternatively check if getWideningDecision's agree, thereby also covering CM_Interleave case?

Outline into `Split = !CM.dicisionsAgree(Instr, VF, VF*2)` ?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8755
+          // decisions agree.
+          if (!Widened) {
+            Split |=
----------------
`if (!Widen && !Split) {` ?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8759
+            Split |= CM.isUniformAfterVectorization(&I, VF) !=
+                     CM.isUniformAfterVectorization(&I, VF * 2);
+          }
----------------
Also need to make sure isScalarWithPredication() are identical?
Originally affects clamping in tryToWidenCall() and handleReplication().


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8808
     VF = SubRange.End;
+    assert(Ranges[I].Start == SubRange.Start && Ranges[I].End == SubRange.End &&
+           "clamped range should match computed range");
----------------
This calls for a Range::operator==, but is needed only temporarily, right?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:73
   // A power of 2.
-  const ElementCount Start;
+  ElementCount Start;
 
----------------
Is this change needed?
Update above comment given that Start is also adjustable?


================
Comment at: llvm/test/Transforms/LoopVectorize/X86/phis-only.ll:6
+
+; CHECK: vector.body:
+define void @test() {
----------------
Worth mentioning the purpose of the test, possibly by renaming the function name?
%0 is dead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98849