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

[SYCL][LIBCLC] Remove target features attributes when compiling for AMD #11247

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

jchlanda
Copy link
Contributor

AMDGPU remove incompatible functions pass replaces all uses of functions that use GPU features incompatible with the current GPU with null then deletes the function. This didn't use to cause problems, as all of libclc functions were inlined prior to incompatible functions pass. Now that the inliner runs later in the pipeline we have to remove all of the target features, so libclc functions will not be earmarked for deletion.

NOTE: I've also reverted the pulldown revert commit to make sure that the original error condition is recreated, hence this PR should not be squashed prior to merging.

Fixes: #10816

@jchlanda jchlanda requested review from a team as code owners September 21, 2023 07:02
@jchlanda jchlanda requested a review from ldrumm September 21, 2023 07:02
@jchlanda
Copy link
Contributor Author

cc: @jsji

@jchlanda jchlanda force-pushed the jakub/pulldown_fix_libclc branch from 16a4e66 to 5d80ab2 Compare September 21, 2023 07:05
@jchlanda jchlanda temporarily deployed to WindowsCILock September 21, 2023 07:10 — with GitHub Actions Inactive
@maksimsab
Copy link
Contributor

Is it just a cherry-pick of https://reviews.llvm.org/D155987 ?

@jchlanda
Copy link
Contributor Author

Is it just a cherry-pick of https://reviews.llvm.org/D155987 ?

No, the commit you posted got reverted - as it uncovered a bug in libclc - so that the pull down could be finished. See: https://github.com/search?q=repo%3Aintel%2Fllvm+Move+placement+of+RemoveIncompatibleFunctions&type=commits

In this patch I provide the fix for libclc: 5d80ab2dcc16acc458d8b26dcc2b77a93e05195d
And revert the revert: 3f6b08bdaf70fcd4ef4ed04c805f8565f04cec6f

@jchlanda jchlanda force-pushed the jakub/pulldown_fix_libclc branch from 5d80ab2 to 2ee0213 Compare September 27, 2023 06:55
@jchlanda jchlanda temporarily deployed to WindowsCILock September 27, 2023 06:56 — with GitHub Actions Inactive
@jchlanda
Copy link
Contributor Author

Friendly ping @intel/llvm-reviewers-runtime / @intel/llvm-reviewers-cuda, are you happy with the fix and reverting the revert commit?

@jchlanda jchlanda temporarily deployed to WindowsCILock September 27, 2023 07:41 — with GitHub Actions Inactive
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.

Approving pipeline changes from DPC++ Tools perspective.

Thanks

@jchlanda jchlanda force-pushed the jakub/pulldown_fix_libclc branch from 2ee0213 to 270d696 Compare October 4, 2023 10:23
@jchlanda jchlanda temporarily deployed to WindowsCILock October 4, 2023 10:25 — with GitHub Actions Inactive
@jchlanda jchlanda temporarily deployed to WindowsCILock October 4, 2023 11:16 — with GitHub Actions Inactive
@jchlanda
Copy link
Contributor Author

jchlanda commented Oct 4, 2023

@intel/llvm-gatekeepers this should be ready to go, I'm not sure how to best handle the revert commit, let me know if you'd like me to submit it in a separate PR and drop it from this one, as squashing both of the commits would not make sense here.

Copy link
Contributor

@ldrumm ldrumm left a comment

Choose a reason for hiding this comment

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

Sorry/ Missed the notification for this one!

Looks good.

RE: squashing: Yes, your commits should be separate. Hopefully a maintainer can do this without the rigmarole of creating multiple PRs that go in at different times.

@aelovikov-intel
Copy link
Contributor

Hopefully a maintainer can do this without the rigmarole of creating multiple PRs that go in at different times.

Nope, please create two PRs.

Copy link
Contributor

@aelovikov-intel aelovikov-intel left a comment

Choose a reason for hiding this comment

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

Request changes to avoid accidental merge without splitting into two PRs.

@jchlanda jchlanda force-pushed the jakub/pulldown_fix_libclc branch from 270d696 to 9811013 Compare October 5, 2023 07:32
@jchlanda jchlanda temporarily deployed to WindowsCILock October 5, 2023 07:33 — with GitHub Actions Inactive
@jchlanda
Copy link
Contributor Author

jchlanda commented Oct 5, 2023

Request changes to avoid accidental merge without splitting into two PRs.

I've dropped the revert commit from this PR (had to rebase/foce push) and opened a new PR with just the revert here: #11438

Thanks for the help.

@aelovikov-intel aelovikov-intel merged commit 881934c into intel:sycl Oct 5, 2023
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.

SYCL passes depends on AMDGPU Remove Incompatible Functions unexpectedly
6 participants