Skip to content

Conversation

@Mel-Chen
Copy link
Contributor

This patch adds runtime test case for vectorization of FindLastIV reduction idiom:

int32_t Rdx = -1;
for (int32_t I = 0; I < TC; I++) {
  Rdx = A[I] > B[I] ? I : Rdx;
}
return Rdx;

Improving test coverage for llvm/llvm-project#67812 and llvm/llvm-project#120395.

Copy link
Member

@alexey-bataev alexey-bataev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but wait for couple days for others to review

@Mel-Chen
Copy link
Contributor Author

Mel-Chen commented Jan 3, 2025

@fhahn @Meinersbur Do you have any suggestions and comments?

Copy link
Contributor

@fhahn fhahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as for, do we need smaller types to trigger epilogue vectorization on more platforms?

IIUC llvm/llvm-project#120395 will need to be fixed first, otherwise the new tests will fail?

@Mel-Chen
Copy link
Contributor Author

Mel-Chen commented Jan 8, 2025

Same as for, do we need smaller types to trigger epilogue vectorization on more platforms?

Sure, could you take a look at AnyOf #195 to see if the approach is appropriate? I will modify this patch in the same way if you feel good.

IIUC llvm/llvm-project#120395 will need to be fixed first, otherwise the new tests will fail?

Yes, the current implementation still produces incorrect output in the following datasets after enabling epilogue vectorization.

    // Check with only Src1[N - 1] > Src2[N - 1].
    for (unsigned I = 0; I != N; ++I)
      Src1[I] = Src2[I] = std::numeric_limits<Ty>::min();
    Src1[N - 1] = std::numeric_limits<Ty>::max();

llvm/llvm-project#120395 can fix this error.

@Mel-Chen
Copy link
Contributor Author

Add test for FFindLastIV, and s16 type for triggering epilogue vectorization easier.
Please take a look again, thanks.

@Mel-Chen Mel-Chen requested a review from fhahn January 14, 2025 08:50
Copy link
Contributor

@fhahn fhahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@Mel-Chen Mel-Chen merged commit d694427 into llvm:main Jan 23, 2025
1 check passed
@AaronBallman
Copy link
Contributor

FYI, but this test fails with GCC 15.x because they enabled -D_GLIBCXX_ASSERTIONS=1 libstdc++ by default. Here's a reproducer on godbolt: https://godbolt.org/z/qh4EGo3s7

We ran into this with our downstream at Intel because we also enable _GLIBCXX_ASSERTIONS, but upstream Clang does not currently do that nor does libc++ currently have the same assertions, so that's likely why we've not started hitting this failure in upstream's tree yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants