[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/LoopVectorize.cpp:1233
+  /// \return An upper bound for the vectorization factor (both fixed and
+  /// scalable) or None if vectorization and interleaving should be avoided up
+  /// front.
----------------
This does not return none any more? It returns a pair with both set to 0. Perhaps we can just say `Returns an upper bound for the vectorization factors (both fixed and scalable). If both factors are 0, vectorization and interleaving should be avoided up front.`


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5817
+  MaxVFCandidates MaxFactors = computeFeasibleMaxVF(TC, UserVF);
+  if (MaxFactors.FixedVF.isVector() && !MaxFactors.ScalableVF) {
+    ElementCount MaxFixedVF = MaxFactors.FixedVF;
----------------
sdesmalen wrote:
> fhahn wrote:
> > 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
> Yes, that's correct, that's something to fix in a follow-up patch. I actually have a draft patch that addresses this, but I didn't want to complicate this patch any further. I've added a FIXME as you suggested.
Sounds good. Would it be possible to add a test case for that scenario?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98721