[PATCH] D98721: [LV] NFC: Return both fixed and scalable Max VF from computeMaxVF.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:208
+/// vectorize with.
+struct MaxVFCandidates {
+  ElementCount FixedVF;
----------------
I’’m not sure what the candidates refers to in the name and I couldn’t spot any reference in any of the comments. AFAICT it’s just a pair of VFs, so perhaps there’s no need to make special reference to the fact that it is used for the maximum VFs?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:221
+
+  static MaxVFCandidates None() { return MaxVFCandidates(); }
+
----------------
Would be good to address?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1234
   /// vectorization and interleaving should be avoided up front.
-  Optional<ElementCount> computeMaxVF(ElementCount UserVF, unsigned UserIC);
+  MaxVFCandidates computeMaxVF(ElementCount UserVF, unsigned UserIC);
 
----------------
Comment needs updating


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1633
   /// to cost.
-  ElementCount computeFeasibleMaxVF(unsigned ConstTripCount,
-                                    ElementCount UserVF);
+  MaxVFCandidates computeFeasibleMaxVF(unsigned ConstTripCount,
+                                       ElementCount UserVF);
----------------
Comment needs updating


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5730
+    if (MaxVF.isScalable()) {
+      MaxScalableVF = MaxVF;
       LLVM_DEBUG(dbgs() << "LV: Found feasible scalable VF = " << MaxVF
----------------
Do we need the separate variables or could we just use the VF pair and adjust the scalable/fixed VFs as we go along?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5817
+  MaxVFCandidates MaxFactors = computeFeasibleMaxVF(TC, UserVF);
+  if (MaxFactors.FixedVF.isVector() && !MaxFactors.ScalableVF) {
+    ElementCount MaxFixedVF = MaxFactors.FixedVF;
----------------
I guess this would pessimize the case where fixed width vectorisation would not require a scalar tail, if a scalable VF is present? Might be good to have a comment/TODO? Also might be good to move the comment that this checks if a tail is required up as well


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98721