This patch returns an Invalid cost from getInstructionCost() for alloca
instructions if the VF is scalable, as otherwise loops which contain
these instructions will crash when attempting to scalarize the alloca.
Details
Diff Detail
Unit Tests
Event Timeline
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
7896 | Can you add a comment describing why we don't want to vectorize this for scalable vectors? | |
llvm/test/Transforms/LoopVectorize/AArch64/scalable-alloca.ll | ||
30 | Can you force the loop to be vectorized with a scalable VF using a loop-hint? With the current RUN line and without hints, the loop may make the decision for other reasons (like considering a fixed-VF cheaper, rather than outright ignoring a scalable VF that was otherwise forced, but had an invalid cost). It may also be worth rebasing your patch on D105806 and adding a CHECK line for one of the remarks (although the remarks may still change depending on review feedback) |
llvm/test/Transforms/LoopVectorize/AArch64/scalable-alloca.ll | ||
---|---|---|
33 | Do we need this alloca and the call to @foo at the bottom? I wonder if it may be simpler to pass a i32** %vla pointer to the function? I think the main thing we care about is the alloca in the loop I think? | |
34 | nit: I think we can simplify the test here by just branching directly, i.e. br label %for.body without the icmp. |
Changes to scalable-alloca.ll:
- Added a loop hint to force vectorization with a scalable VF
- Rebased the patch and added CHECK lines for Invalid costs found in the loop (from D105806)
- Passed i32** %vla to the @alloca function
llvm/test/Transforms/LoopVectorize/AArch64/scalable-alloca.ll | ||
---|---|---|
2 | Makes sense to use the python file to create the check lines here? |
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
7898 | nit: this line is redundant, because the code says as much. | |
llvm/test/Transforms/LoopVectorize/AArch64/scalable-alloca.ll | ||
2 | @CarolineConcatto I don't think that is necessary for this specific patch because this test is not guarding the fixed-width vectorization capabilities of the LV, nor the AArch64 CostModel. | |
4 | Can you also add: CHECK-REMARKS: UserVF ignored because of invalid costs. | |
9 | I'd suggest to remove these CHECK lines, because the fact that this loop vectorizes with fixed-width vectors is down to the cost-model, which means we may have to regenerate the check lines at some point, even though they're not needed for what it is that you're trying to test. |
- Removed unnecessary CHECK lines from scalable-alloca.ll
- Added a check for the "UserVF ignored because of invalid costs" remark
LGTM, thanks.
Just FYI, you'll want to wait with landing it until I've relanded D105806.
Can you add a comment describing why we don't want to vectorize this for scalable vectors?