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.

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

[SPIR-V] W/A for builtin vars translation #7632

Merged
merged 4 commits into from
Dec 8, 2022

Conversation

MrSidims
Copy link
Contributor

@MrSidims MrSidims commented Dec 4, 2022

The translator was crashing in case if builin GV was accessed via GEP without AS cast due to incorrect assumption. The W/A replaced GEP with ExtractElementInst.

This W/A is exclusive for intel/llvm . Proper fix is done in SPIR-V consumer, but it will take take time to have the drivers be updated in CI.

Signed-off-by: Sidorov, Dmitry [email protected]

@MrSidims MrSidims marked this pull request as ready for review December 4, 2022 19:01
@MrSidims MrSidims requested a review from a team as a code owner December 4, 2022 19:01
@MrSidims
Copy link
Contributor Author

MrSidims commented Dec 4, 2022

/summary:run

@MrSidims MrSidims requested review from bader and Fznamznon December 4, 2022 19:17
@MrSidims
Copy link
Contributor Author

MrSidims commented Dec 4, 2022

/summary:run

The translator was crashing in case if builin GV was accessed via
GEP without AS cast due to incorrect assumption.

This W/A is exclusive for intel/llvm

Signed-off-by: Sidorov, Dmitry <[email protected]>
Signed-off-by: Sidorov, Dmitry <[email protected]>
@MrSidims
Copy link
Contributor Author

MrSidims commented Dec 6, 2022

Need to add 51a49d2 as well

Signed-off-by: Sidorov, Dmitry <[email protected]>
@MrSidims
Copy link
Contributor Author

MrSidims commented Dec 6, 2022

@asudarsa please take a look. This issue affects pulldown.

@MrSidims
Copy link
Contributor Author

MrSidims commented Dec 6, 2022

Proper testing is done in #7637
CPU tests there are passing now

@MrSidims MrSidims requested a review from a team December 7, 2022 09:58
@MrSidims
Copy link
Contributor Author

MrSidims commented Dec 7, 2022

@asudarsa friendly ping

Copy link
Contributor

@asudarsa asudarsa 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. Minor nit to add link to issue as part of the code comment.

@MrSidims
Copy link
Contributor Author

MrSidims commented Dec 8, 2022

@intel/llvm-gatekeepers please merge

@steffenlarsen steffenlarsen merged commit ead8404 into intel:sycl Dec 8, 2022
vmaksimo added a commit that referenced this pull request Jul 9, 2024
dm-vodopyanov pushed a commit that referenced this pull request Jul 10, 2024
This reverts commit ead8404.

The commit was W/A which should be removed to align with Khronos
translator code base.
It resolves p.2 of #7592 "adds extra code" section.
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