[PATCH] D96011: [NFC][Analysis] Change struct VecDesc to use ElementCount

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:604-607
+    for (unsigned VF = 2; VF <= WidestScalableVF; VF *= 2) {
+      Scalarize &= !TLI.isFunctionVectorizable(ScalarName,
+                                               ElementCount::getScalable(VF));
+    }
----------------
sdesmalen wrote:
> This can probably return false directly when WidestScalableVF is > 0, because there is no way to scalarize for scalable VFs.
I thought about that as well when I first did the patch, but I don't think that's right. This function is not asking if something **should ** be scalarised, but instead asking if there is at least one variant of the function that is vectorisable. If we return false here when WidestScalableVF > 0 then that's a bug I think because that means we're saying we can definitely vectorise at least one function, which may not be true. As an alternative I could add an assert like this:

assert(WidestScalableVF.isZero() || !Scalarize);

Also, returning true from this function doesn't mean we are going to scalarise using as a scalable VF, even if widest fixed and scalable VFs are both non-zero. It just means that no vectorisable variants could be found and therefore the caller must decide what to do and which variant to scalarise - fixed or scalable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96011