[PATCH] D143938: [VPlan] Compute costs for plans directly after construction.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:283
+      std::pair<VPlanPtr, SmallVector<std::pair<VectorizationFactor, bool>>>, 4>
+      VPlans;
 
----------------
Ayal wrote:
> Perhaps the following would be better, at-least when first moving VectorizationFactors from CM to LVP: keep `VPlans` as a vector of all VPlanPtrs, as its name suggests; hold all VectorizationFactors in a separate vector or set indexed by their ElementCount Width (as in LVP::plan()'s CandidateVFs)? Can also hold a map associating each VectorizationFactor with its VPlan, if needed; is the reverse mapping useful?
> 
> (Independent of this patch): Hold the boolean IsVec as a field inside VectorizationFactor, rename VectorizationFactor more accurately.
Thanks, I updated the code to use a separate mapping from VPlan->vector of pair<VectorizationFactor, bool>. This ensures that the costs are associated with a specific VPlan. WDYT?

As for the IsVec field, I am going to investigate if this is actually needed or if we can get the information from the existing VPlan.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:287
   VPBuilder Builder;
 
+  using InstructionVFPair = std::pair<Instruction *, ElementCount>;
----------------
Ayal wrote:
> /// ...
Not needed in the latest version, I removed the code.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:347-350
+  /// \return The most profitable vectorization factor and the cost of that VF.
+  /// This method checks every VF in \p CandidateVFs. If UserVF is not ZERO
+  /// then this vectorization factor will be selected if vectorization is
+  /// possible.
----------------
Ayal wrote:
> nit: comment deserves update - CandidateVFs is no longer a parameter. (Independent of this patch:) UserVF is irrelevant, behavior of ForceVectorization should be explained.
Thanks, updated the comment!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5513
+      return !isMoreProfitable(Candidate.first, ChosenFactor);
+    });
 
----------------
Ayal wrote:
> This is independent of Plan, right?
Yes. I think actually filtering out the unprofitable VFs is not really needed, as the loop below should handle all cases already. Removed the code. Could arguably also be split off.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5659
+           ElementCount::isKnownLT(NextVF.Width, MainLoopVF)) &&
+          (Result.Width.isScalar() || isMoreProfitable(NextVF, Result)))
+        Result = NextVF;
----------------
Ayal wrote:
> nit: can assert there's a Plan that has NextVF.Width?
Added, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8752
+        "ConditionalStore", ORE, OrigLoop);
+    VPlans.clear();
+  }
----------------
Ayal wrote:
> nit: also prevents unrolling, hence no need to retain even the scalar VPlan of VF=1 for unrolling UF>1.
Will update this independently.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143938