[clang][SYCL] Transfer opt level to clang target options #18470
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
omarahmed1111
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
omarahmed1111
force-pushed
the
transfer-opt-level-to-clang-target-options
branch
from
a9796d4 to
63f3e57
Compare
omarahmed1111
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
omarahmed1111
force-pushed
the
transfer-opt-level-to-clang-target-options
branch
from
63f3e57 to
81813a1
Compare
omarahmed1111
had a problem deploying
to
WindowsCILock
GitHub Actions
Error
omarahmed1111
force-pushed
the
transfer-opt-level-to-clang-target-options
branch
from
81813a1 to
082f04b
Compare
omarahmed1111
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
omarahmed1111
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
omarahmed1111
had a problem deploying
to
WindowsCILock
GitHub Actions
Failure
omarahmed1111
force-pushed
the
transfer-opt-level-to-clang-target-options
branch
from
082f04b to
09fa1a9
Compare
omarahmed1111
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
dm-vodopyanov
changed the title
omarahmed1111
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
omarahmed1111
temporarily deployed
to
WindowsCILock
GitHub Actions
Inactive
mdtoguchi
reviewed
May 21, 2025
| } | ||
| } | ||
| CC1Args.push_back(DriverArgs.MakeArgString(llvm::Twine("-O") + OOpt)); | ||
| } |
There was a problem hiding this comment.
Does it make sense to make this a common function as it is also used for setting the opt level for the assembler?
| .Default("2"); | ||
| } | ||
| } | ||
| CC1Args.push_back(DriverArgs.MakeArgString(llvm::Twine("-O") + OOpt)); |
There was a problem hiding this comment.
Please add a test to cover the passing of the optimization setting and the default expectation.
|
@mdtoguchi I actually solved this problem wrong, the solution was not in this PR, but the underneath problem was that adding any potential early return could skip the opt level transfer. I added a PR to solve that upstream here. I think we shouldn't add that here as |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, without transferring the opt level to clang target options if we called:
it will not transfer this
-O3to the CC1 args underneath that compiles the nvptx kernels:This PR would make it more consistent by explicitly transfer driver opt level called to CC1 args:
The PR also make
-O3the default opt level. I tried to mimic the default opt level passed here.