This patch enables scalable vectors in the VPlan-native path.
If a vectorization factor is specified via loop vectorization hints,
that factor is used. If no vectorization factor is specified, but the
target preferes scalable vectorization, a scalable vectorization factor
is selected.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Looks reasonable overall, my only concern is that this adds another codepath to the native path which is already lacking test coverage in general. Do you have any idea/suggestion on how to improve end-to-end test coverage, e.g. in llvm-test-suite (could be adding some SingleSource tests with loop nests and #pragma clang loop vectorize_width(4)?
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
7385 | Can a test be added for this case? |
Add check for optimization remarks and rejection of scalable VFs on targets without scalable vectors.
Thank you very much for your reply @fhahn! I have a private fork of the llvm-test-suite where I have something similar to this: A subdirectory VPlanNativePath in llvm-test-suite/SingleSource/UnitTests/Vectorizer/ that is only added to the CMake build if the compiler is clang. In that directory, I also add -mllvm -enable-vplan-native-path to the CMAKE_C_FLAGS and it contains simple tests that initialise some 1d and 2d arrays, does some operations on them (with loops that have #pragma clang loop vectorize(enable) in front of them), and prints the result arrays to stdout (for checking against a reference). If this general approach would be fine with you, I will happily submit patches with the tests that already outer-loop-vectorize with the current upstream functionality!
Might I assume your judgement on D157371 (#pragma [...] interleave_count(2) for the VPlan-native path) is similar to the comment here?
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
7385 | Thank you for the feedback, I added a second RUN: line to llvm/test/Transforms/LoopVectorize/outer_loop_scalable.ll where scalable vectors are not enabled and added CHECK lines for the optimization remarks. |
Sorry for the delay, here it is: https://github.com/llvm/llvm-test-suite/pull/20 ! I did not find a way to request reviewers (the usual link was greyed out for me) in the GitHub PR, so sorry for using a comment to tag you there.
LGTM, thanks!
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
7346–7347 | might be good to document here that this will pick a scalable VF if target supports scalable vectors or fixed one otherwise (before TODO) |
I didn't realize this hasn't landed yet. @iamlouk would you like me to commit this on your behalf? If so, please let me know the name + email to use to set as author of the commit.
Great, will do! Unfortunately it looks like this doesn't apply cleaning. Could you rebase it?
Great, will do! Unfortunately it looks like this doesn't apply cleaning. Could you rebase it?
might be good to document here that this will pick a scalable VF if target supports scalable vectors or fixed one otherwise (before TODO)