This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Support scalable vectors in outer-loop vectorization
ClosedPublic

Authored by iamlouk on Aug 9 2023, 2:25 AM.

Details

Summary

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.

Diff Detail

Event Timeline

iamlouk created this revision.Aug 9 2023, 2:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2023, 2:25 AM
iamlouk requested review of this revision.Aug 9 2023, 2:25 AM
iamlouk updated this revision to Diff 553888.Aug 28 2023, 4:15 AM

Rebase and add call to reportVectorizationFailure()

iamlouk updated this revision to Diff 554174.Aug 28 2023, 10:00 PM

Fix clang-format

fhahn added a comment.Aug 30 2023, 8:34 AM

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
7521

Can a test be added for this case?

iamlouk updated this revision to Diff 554941.Aug 31 2023, 2:09 AM

Add check for optimization remarks and rejection of scalable VFs on targets without scalable vectors.

iamlouk marked an inline comment as done.Aug 31 2023, 2:21 AM

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)?

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
7521

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.

Matt added a subscriber: Matt.Aug 31 2023, 10:42 AM
fhahn added a comment.Sep 5 2023, 7:24 AM

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)?

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?

That sounds like a good first step, can you share a patch for that?

iamlouk marked an inline comment as done.Sep 8 2023, 1:34 AM

That sounds like a good first step, can you share a patch for that?

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.

fhahn accepted this revision.Sep 22 2023, 4:27 AM

LGTM, thanks!

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7482–7483

might be good to document here that this will pick a scalable VF if target supports scalable vectors or fixed one otherwise (before TODO)

This revision is now accepted and ready to land.Sep 22 2023, 4:27 AM
iamlouk updated this revision to Diff 557498.Sep 29 2023, 1:25 PM

Rebase and add comment on VF selection.

fhahn added a comment.Oct 19 2023, 8:04 AM

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.

Yes, thank you: Lou Knauer <lou.knauer@sipearl.com>

fhahn added a comment.Oct 20 2023, 4:11 AM

Yes, thank you: Lou Knauer <lou.knauer@sipearl.com>

Great, will do! Unfortunately it looks like this doesn't apply cleaning. Could you rebase it?

fhahn added a comment.Oct 20 2023, 4:22 AM

Yes, thank you: Lou Knauer <lou.knauer@sipearl.com>

Great, will do! Unfortunately it looks like this doesn't apply cleaning. Could you rebase it?

This revision was automatically updated to reflect the committed changes.