-
Notifications
You must be signed in to change notification settings - Fork 349
[SingleSource/Vectorizer] Add unit tests for the vplan-native path. #20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This patch adds unit tests for LLVM's VPlan-native path, as requested in https://reviews.llvm.org/D157484. The new Vectorizer/VPlanNativePath subdirectory is only enabled if the compiler is clang. For all source files in that directory, the flags "-mllvm -enable-vplan-native-path" are added. Four different scenarios are tested for outer-loop vectorization: - Matrix multiplication, where the second of three loops is vectorized. - A test where the vectorized loop has an auxiliary induction variable. - A test for indirect and strided memory accesses. - A nesting of three loops where the outer-most one is vectorized.
I do not seam to be able to add reviewers, so sorry for tagging you here in a comment: @fhahn @alexey-bataev Please also note that the matrix multiplication test will cause current LLVM to crash without https://reviews.llvm.org/D150700 which is not yet merged/accepted. |
Sorry for the delay with reviewing this! Trying to build this with the latest version of
I'll get a reproducer. |
No worries, thank you for having a look. I cannot confirm if its exactly that assert that breaks because I do not have my computer near by, but I know that the bug fixed in this patch is triggered by one of the tests: https://reviews.llvm.org/D150700 ("[LV] Stability fix for outerloop vectorization") by @nikolaypanchenko (@npanchen ? Sorry if tagging a random stranger, GitHub suggests this username). |
Indeed, looks like the patch should help, but would need a rebase. |
@iamlouk could you check if the new tests now build and run without issues after llvm/llvm-project#68118 landed? |
Hello, yes, the tests compile and pass now! I tested with llvm-project commit 88f0e4c75c1ac498f2223fc640c4ff6c572c5ed1, the llvm-test-suite was built using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
An additional suggestion about keeping the variable names consistent inline.
DEFINE_SCALAR_AND_VECTOR_FN_FOR_NESTED_OLV( | ||
(size_t N, size_t M, size_t L, | ||
int32_t *__restrict__ A, const int32_t *B, const int32_t *C), | ||
for (size_t j = 0; j < L; j++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to consistently use upper case for variable names, as per LLVM coding style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for this, I fixed that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the latest update, still LGTM!
Sorry to ask you for this, but could you merge this for me? I don't have the permissions. Thank you very much in advance. |
Hi @iamlouk I find that this test causes compliation crash on x86-64 with clang-18 O2:
Did I miss something? |
@SixWeining interesting, could you share the IR causing the crash? ( |
Uploaded: crash.ll.txt Thanks. |
Note that llvm is built with |
@SixWeining not sure how expensive checks impact the codegen, but the issue should be fixed by a04f6152914ea21f3068aaba9d8fc21d2e703d3e |
Unfortunately, still crash with the
|
Hello, I think this crash is just because of this condition in @fhahn , the changes you made in a04f6152 are what could be needed to be able to remove the "-fno-vectorize" I added here, but have not confirmed that yet. |
This patch adds unit tests for LLVM's VPlan-native path, as requested in https://reviews.llvm.org/D157484.
The new Vectorizer/VPlanNativePath subdirectory is only enabled if the compiler is clang. For all source files in that directory, the flags "-mllvm -enable-vplan-native-path" are added.
Four different scenarios are tested for outer-loop vectorization: