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

[UR][L0] Make urPlatformGetBackendOption return -ze-opt-level=2 for -O1 and -O2 #1129

Merged
merged 2 commits into from
Jan 10, 2024

Conversation

sarnex
Copy link
Contributor

@sarnex sarnex commented Nov 27, 2023

IGC only accepts three values for -ze-opt-level, 0, 1 and 2, however dpcpp has at least four options, O0, O1, O2 and O3.

Right now we decided to map O1 and O2 to -ze-opt-level=1, but we have determined that this is not the best for user experience and may defy user expectation, and actually produces worse code than specifying nothing because IGC uses ze-opt-level=2 if nothing is specified.

intel/llvm#12023

@sarnex sarnex marked this pull request as ready for review November 27, 2023 17:33
@sarnex sarnex requested a review from a team as a code owner November 27, 2023 17:33
@sarnex
Copy link
Contributor Author

sarnex commented Nov 27, 2023

@kbenzie Do you mind approving the CI run for this? Thanks!

Copy link

@rakeshkrishnaiyer rakeshkrishnaiyer left a comment

Choose a reason for hiding this comment

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

lgtm

@jandres742
Copy link

thanks @sarnex . could you open a PR in intel/llvm to validate the changes before merging? please see guidelines here https://oneapi-src.github.io/unified-runtime/core/CONTRIB.html#adapter-change-process and a sample here

github.com//pull/1099
github.com/intel/llvm/pull/11958

@sarnex sarnex changed the title [UR][L0] Make urPlatformGetBackendOption return -ze-opt-level=2 for -O2 [UR][L0] Make urPlatformGetBackendOption return -ze-opt-level=2 for -O1 and -O2 Nov 28, 2023
sarnex added a commit to sarnex/llvm that referenced this pull request Nov 28, 2023
@jandres742
Copy link

changes are looking good. We will wait for llvm testing.

@sarnex
Copy link
Contributor Author

sarnex commented Nov 28, 2023

@jandres742 Sorry about that, the contribution guidelines changed since I last worked here and I made the mistake of assuming I didn't need to check again :)

The llvm PR CI passed, so we should be good there

@jandres742
Copy link

thanks @sarnex

@kbenzie : please merge when possible. Not mandatory 0.8.x.

@kbenzie kbenzie added the ready to merge Added to PR's which are ready to merge label Nov 29, 2023
@fabiomestre fabiomestre changed the base branch from adapters to main December 5, 2023 16:45
@fabiomestre
Copy link
Contributor

I have updated the target branch of this PR from the adapters branch to the main branch.
Development in UR is moving back to main. The adapters branch will soon be deleted.

@sarnex
Copy link
Contributor Author

sarnex commented Dec 5, 2023

Sure, hopefully we can merge this soon, let me know if something is needed from me

@sarnex
Copy link
Contributor Author

sarnex commented Jan 8, 2024

@fabiomestre @kbenzie Any ETA on this being merged? Thanks

@kbenzie
Copy link
Contributor

kbenzie commented Jan 8, 2024

@fabiomestre @kbenzie Any ETA on this being merged? Thanks

We're aiming to clear the ready to merge backlog this week.

@sarnex
Copy link
Contributor Author

sarnex commented Jan 8, 2024

@kbenzie Cool thanks for the update

Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>
Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>
@kbenzie kbenzie merged commit c2d7825 into oneapi-src:main Jan 10, 2024
51 checks passed
steffenlarsen pushed a commit to intel/llvm that referenced this pull request Jan 10, 2024
…O1 and -O2 (#12023)

oneapi-src/unified-runtime#1129

---------

Signed-off-by: Sarnie, Nick <nick.sarnie@intel.com>
Co-authored-by: Kenneth Benzie (Benie) <k.benzie@codeplay.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Added to PR's which are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants