Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[SPIR-V] Reland entry point patches #14450

Open
wants to merge 9 commits into
base: sycl
Choose a base branch
from
Open

[SPIR-V] Reland entry point patches #14450

wants to merge 9 commits into from

Conversation

vmaksimo
Copy link
Contributor

@vmaksimo vmaksimo commented Jul 4, 2024 •
edited
Loading

vmaksimo added 2 commits July 4, 2024 07:23
@vmaksimo
Add an entry point wrapper around functions (llvm pass)
86aa271 
SPIR-V spec states:
"It is invalid for any function to be targeted by both an OpEntryPoint
instruction and an OpFunctionCall instruction."

In order to satisfy SPIR-V that entrypoints and functions
must be different, this introduces an entrypoint wrapper around
functions at the LLVM IR level, then fixes up a few things like
naming at the SPIRV translation.

Original commit:
KhronosGroup/SPIRV-LLVM-Translator@85815e7
@vmaksimo
Translate function attributes (incl. parameter attrs) for entry point…
f5f4cc5 
… kernels

Original commit: KhronosGroup/SPIRV-LLVM-Translator@aded5af
@vmaksimo vmaksimo temporarily deployed to WindowsCILock July 4, 2024 15:18 — with GitHub Actions Inactive
@vmaksimo vmaksimo changed the title [SPIR-V] Restore Entry Point and its attributes translation [SPIR-V] Reland entry point patches Jul 4, 2024
@vmaksimo vmaksimo temporarily deployed to WindowsCILock July 4, 2024 17:18 — with GitHub Actions Inactive
@vmaksimo vmaksimo temporarily deployed to WindowsCILock July 4, 2024 17:20 — with GitHub Actions Inactive
@vmaksimo vmaksimo marked this pull request as ready for review July 8, 2024 12:46
@vmaksimo vmaksimo requested a review from a team as a code owner July 8, 2024 12:46
@vmaksimo vmaksimo temporarily deployed to WindowsCILock July 8, 2024 13:22 — with GitHub Actions Inactive
@vmaksimo vmaksimo temporarily deployed to WindowsCILock July 8, 2024 13:29 — with GitHub Actions Inactive
@vmaksimo vmaksimo temporarily deployed to WindowsCILock July 9, 2024 10:08 — with GitHub Actions Inactive
@vmaksimo vmaksimo temporarily deployed to WindowsCILock July 9, 2024 10:09 — with GitHub Actions Inactive
@vmaksimo
Copy link
Contributor Author

vmaksimo commented Jul 9, 2024

clang lit failure seems to be not related to the change (at least it should not be).

@vmaksimo vmaksimo temporarily deployed to WindowsCILock July 9, 2024 10:58 — with GitHub Actions Inactive
@vmaksimo vmaksimo temporarily deployed to WindowsCILock July 9, 2024 10:59 — with GitHub Actions Inactive
@LU-JOHN
Copy link
Contributor

LU-JOHN commented Jul 9, 2024

Do we know why these changes were not pulled down into intel/llvm's llvm-spirv to begin with? Did something change so that it is ok now?

@vmaksimo
Copy link
Contributor Author

vmaksimo commented Jul 9, 2024

@LU-JOHN yes, we waited for GPU (Windows?) driver to be updated.

@LU-JOHN
Copy link
Contributor

LU-JOHN commented Jul 9, 2024

@vmaksimo Are there any differences from Khronos that we should focus our review on?

@vmaksimo
Copy link
Contributor Author

vmaksimo commented Jul 9, 2024

@LU-JOHN there are code differences from the initial commit changes (first two commits). But after this PR there will be no differences between Khronos code base and intel/llvm (regarding the mentioned in the description changes).

LU-JOHN
LU-JOHN approved these changes Jul 9, 2024
View reviewed changes
Copy link
Contributor

@LU-JOHN LU-JOHN left a comment

Choose a reason for hiding this comment

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

LGTM

@vmaksimo
Copy link
Contributor Author

Hi @intel/llvm-gatekeepers! Could you please merge?
The failure in Clang :: Driver/sycl-linker-wrapper-image.cpp seems unrelated to the change.

@steffenlarsen
Copy link
Contributor

@vmaksimo - The failure does indeed seem related and I was able to reproduce it locally. Changing all cases of 772 to 912 in the test seems to fix it. @asudarsa | @AlexeySachkov, what are the implications of such a change?

AlexeySachkov
AlexeySachkov requested changes Jul 11, 2024
View reviewed changes
llvm-spirv/test/extensions/INTEL/SPV_INTEL_function_pointers/alias.ll Show resolved Hide resolved
llvm-spirv/test/image.ll Outdated
@@ -1,5 +1,3 @@
; XFAIL: *
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, the test passes in the upstream translator repo, if we are going to disable any of tests here, we need to submit a tracker to investigate the failures and re-enable those tests.

@vmaksimo vmaksimo requested a review from AlexeySachkov July 26, 2024 11:44
@vmaksimo
Copy link
Contributor Author

vmaksimo commented Aug 6, 2024

Re-trigger CI

@vmaksimo vmaksimo closed this Aug 6, 2024
@vmaksimo vmaksimo reopened this Aug 6, 2024
@vmaksimo vmaksimo requested a review from a team as a code owner January 10, 2025 11:09
@vmaksimo
Copy link
Contributor Author

undef issue catched by format check will be addressed in KhronosGroup/SPIRV-LLVM-Translator#2953.
SYCL :: USM/fill_any_size.cpp failure is not related, it's failing in other PRs too.

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

@LU-JOHN LU-JOHN LU-JOHN approved these changes

@MrSidims MrSidims MrSidims approved these changes

@AlexeySachkov AlexeySachkov AlexeySachkov approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vmaksimo @LU-JOHN @steffenlarsen @AlexeySachkov @MrSidims