This patch changes the VecDesc struct to use ElementCount
instead of an unsigned VF value, in preparation for
future work that adds support for vectorized versions of
math functions using scalable vectors. Since all I'm doing
in this patch is switching the type I believe it's a
non-functional change. I changed getWidestVF to now return
both the widest fixed-width and scalable VF values, but
currently the widest scalable value will be zero.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/include/llvm/Analysis/VectorUtils.h | ||
|---|---|---|
| 194 ↗ | (On Diff #321366) | Can you pull this change out into a separate patch? Then the remainder of this patch becomes NFC (especially when you add asserts the VF must be fixed-width for the entries). |
| llvm/lib/Analysis/TargetLibraryInfo.cpp | ||
| 1673–1674 | Can you define these as ElementCount (and change the interface to take an ElementCount instead of unsigned), and do the initialization before the call to getWidestVF? | |
| 1681–1684 | nit: ElementCount *Ptr = VF.isScalable() ? &ScalableVF : &FixedVF; if (ElementCount::isKnownGT(VF, *Ptr)) *Ptr = VF; | |
| llvm/lib/Transforms/Utils/InjectTLIMappings.cpp | ||
| 109 | nit: unnecessary curly braces. | |
| 121 | This needs an assert to make sure WidestScalableVF is not set, so that it is not silently ignoring any scalable entries. | |
| llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp | ||
| 604–607 | This can probably return false directly when WidestScalableVF is > 0, because there is no way to scalarize for scalable VFs. | |
| llvm/lib/Analysis/TargetLibraryInfo.cpp | ||
|---|---|---|
| 1673–1674 | Hi @sdesmalen, I did that initially but then I thought it seemed a bit unnecessary to have them as ElementCounts because the interface clearly defines one as fixed and the other as scalable. I can change them back to ElementCount - it just felt like we were doing redundant extra work that's all. | |
| llvm/lib/Analysis/TargetLibraryInfo.cpp | ||
|---|---|---|
| 1673–1674 | What is the redundant extra work? The only difference is in where the initialization is done. Now all uses of FixedVF coming from getWidestVF have to write explicit ElementCount::getFixed(). Just FYI, I had to solve a similar problem for returning max-VFs in D96025, for which I added a new class OptionalVFCandidates, and in a subsequent patch (not yet posted) added a function that generates all power-of-two VFs upto the max-VFs. | |
| llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp | ||
|---|---|---|
| 604–607 | 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? | |
| llvm/lib/Analysis/TargetLibraryInfo.cpp | ||
|---|---|---|
| 1611–1613 | nit: const ElementCount & | |
| llvm/lib/Transforms/Utils/InjectTLIMappings.cpp | ||
| 114 | nit: s/unsupported/not yet supported/ | |
| llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp | ||
| 598 | Should we just add a constructor that takes no arguments and does the same as getNull? | |
| 603–608 | This should be Element::getScalable(1) because <vscale x 1 x <eltty>> may well be a valid type for some scalable vector architecturs. For SVE we don't loops to be vectorized with this VF because it is expensive to legalize, but we shouldn't work with that assumption for generic code. | |
Comment Actions
Thanks for the changes, LGTM.
| llvm/lib/Transforms/Utils/InjectTLIMappings.cpp | ||
|---|---|---|
| 91–94 | nit: can you move this closer to the for loop? | |
nit: const ElementCount &