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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 11 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:208
-  const TargetLibraryInfo *TLI;
-
   /// Target Transform Info.
----------------
Ayal wrote:
> This cleanup is independent?
Unfortunately not at the moment. it is still used during the widening checks in tryToWidenCall I think.


================
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) {
----------------
Ayal wrote:
> Ideally these `bool CM.should*(inst, VF)` methods would be const, but that seems to require various other methods to be const as well?
I pushed a few NFC commits changing the used methods to const . Now `shouldWiden.*` can be const as well.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8685
+  Ranges.emplace_back(MinVF, MaxVF.getWithIncrement(1));
+
+  auto SplitRange = [&Ranges](unsigned Idx, ElementCount VF) {
----------------
Ayal wrote:
> // 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?
Unfortunately it cannot,  because ` Ranges.insert` needs to be able to overwrite existing entries I think.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8714
+
+  for (BasicBlock *BB : make_range(DFS.beginRPO(), DFS.endRPO())) {
+    for (Instruction &I : BB->instructionsWithoutDebug()) {
----------------
Ayal wrote:
> Better scan basic-blocks in regular DFS order, avoid RPO.
I think we can just iterate over the loop blocks. I removed the DFS/RPO.


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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8750
+            Split |=
+                CM.shouldWidenMemory(&I, VF) != CM.shouldWidenMemory(&I, VF);
+            Widened |= CM.shouldWidenMemory(&I, VF);
----------------
Ayal wrote:
> 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)` ?
> (calls for a test?)

yes, but unfortunately I could not construct a test case where it only comes down to whether the decisions agree for a memory operation. for example, it is possible to have a case where we switch from `interleave-group` to `widen`, but in that case the recipe for the `GEP` also changes.


================
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");
----------------
Ayal wrote:
> This calls for a Range::operator==, but is needed only temporarily, right?
yes it's just temporary. I can add `Range::operator==`, but I think it would become unused, after the next patch goes in.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:73
   // A power of 2.
-  const ElementCount Start;
+  ElementCount Start;
 
----------------
Ayal wrote:
> Is this change needed?
> Update above comment given that Start is also adjustable?
Unfortunately yes, because `Ranges.insert` needs to overwrite existing elements.

We should be able to limit modifications once the clamping is gone, but using a vector of `const VFRange`.


================
Comment at: llvm/test/Transforms/LoopVectorize/X86/phis-only.ll:6
+
+; CHECK: vector.body:
+define void @test() {
----------------
Ayal wrote:
> Worth mentioning the purpose of the test, possibly by renaming the function name?
> %0 is dead?
Done, I added a comment and adjusted the check lines to check for a plan including all VFs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98849