Skip to content

[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

Merged
merged 9 commits into from
Dec 12, 2022

Conversation

fineg74
Copy link
Contributor

@fineg74 fineg74 commented Dec 7, 2022

This is to have sycl-post-link to handle IR in the form

%0 = load i64, i64 addrspace(1)* getelementptr (<3 x i64>, <3 x i64> addrspace(1)* @__spirv_BuiltInGlobalInvocationId, i64 0, i64 0), align 32
  store i64 %0, i64 addrspace(1)* %_arg_DoNotOptimize, align 8

@fineg74
Copy link
Contributor Author

fineg74 commented Dec 7, 2022

AMDGPU failures
SYCL :: Assert/assert_in_kernels.cpp
SYCL :: Assert/assert_in_multiple_tus.cpp
SYCL :: Assert/assert_in_multiple_tus_one_ndebug.cpp
SYCL :: Assert/assert_in_one_kernel.cpp
SYCL :: Assert/assert_in_simultaneously_multiple_tus_one_ndebug.cpp

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)
Copy link
Contributor

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.

Copy link
Contributor

@AlexeySachkov AlexeySachkov left a 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.

@pvchupin pvchupin requested a review from v-klochkov December 10, 2022 00:35
@kbobrovs
Copy link
Contributor

Approving and merging due to urgency. But I agree with this:

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.

So, @fineg74 - could you please address this in later PRs?

@kbobrovs kbobrovs merged commit 5de8d26 into intel:sycl Dec 12, 2022
@fineg74 fineg74 deleted the syclPostLinkTrunc branch December 12, 2022 19:41
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.

3 participants