-
Notifications
You must be signed in to change notification settings - Fork 787
[SYCL][ESIMD] Fix crashes of sycl-post-link #7673
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
# Conflicts: # llvm/lib/SYCLLowerIR/ESIMD/LowerESIMD.cpp # llvm/test/tools/sycl-post-link/sycl-post-link-test.ll
AMDGPU failures Are not related to the change |
(isa<ExtractElementInst>(LU) || isa<TruncInst>(LU)) && | ||
"SPIRV global users should be either ExtractElementInst or TruncInst"); | ||
// Ignore pointer type uses | ||
if (LU->getType()->getTypeID() == llvm::Type::TypeID::PointerTyID) |
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.
I think this is too generic. You basically need to distinguish the
%0 = load i64, i64 addrspace(1)* getelementptr (<3 x i64>, <3 x i64> addrspace(1)* @__spirv_BuiltInGlobalInvocationId, i64 0, i64 0), align 32
case from others. This case above performs thread_id calculation in one instruction basically, unlike the other two variants, where load is followed by element extraction or truncation.
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.
The implementation is owned by ESIMD folks, only test is owned by Tools. Approving the test, so this can be merged as soon as implementation is approved.
However, I would recommend to move this test somewhere and perform it through opt
. This way, it would be more like a unit-test for a single pass, because right now we are performing an integration test of the whole sycl-post-link
to test a single pass change.
Approving and merging due to urgency. But I agree with this:
So, @fineg74 - could you please address this in later PRs? |
This is to have sycl-post-link to handle IR in the form