Skip to content

Return the usage of -flto and use -lto-emit-asm instead #16884

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
Merged
merged 3 commits into from
Feb 14, 2025
Merged

Return the usage of -flto and use -lto-emit-asm instead #16884

merged 3 commits into from
Feb 14, 2025

Conversation

@omarahmed1111
Copy link
Contributor

@omarahmed1111 omarahmed1111 commented Feb 4, 2025 •
edited
Loading

This reverts this PR and add another solution to that problem mentioned in this issue. This was the result of llvm upstream discussion here

@omarahmed1111 omarahmed1111 requested review from a team as code owners February 4, 2025 18:01
@omarahmed1111
Copy link
Contributor Author

omarahmed1111 commented Feb 6, 2025

@mdtoguchi @AlexeySachkov The PR is failing in some tests due to not finding clang-nvlink-wrapper tool. I saw locally that the tool should be built by default with other clang tools and it works normally. Do you have an idea where the PATH is initialized on CI or how to debug that?

@omarahmed1111 omarahmed1111 force-pushed the readd-lto-and-fix-the-pipeline-to-generate-ptx branch from d542197 to 83ca7aa Compare February 6, 2025 13:53
@AlexeySachkov
Copy link
Contributor

AlexeySachkov commented Feb 7, 2025

@mdtoguchi @AlexeySachkov The PR is failing in some tests due to not finding clang-nvlink-wrapper tool. I saw locally that the tool should be built by default with other clang tools and it works normally. Do you have an idea where the PATH is initialized on CI or how to debug that?

My guess is that dependency is missing here:

llvm/sycl/test-e2e/CMakeLists.txt

Lines 94 to 98 in c2dc01d

list(APPEND SYCL_E2E_TEST_DEPS
sycl-toolchain
FileCheck
not
)

clang-nvlink-wrapper is not a part of our sycl-toolchain - if it should be (i.e. that is not a test-only dependency), then the following CMakeLists.txt should be updated:

llvm/sycl/CMakeLists.txt

Line 366 in c2dc01d

add_custom_target(sycl-compiler

Hope this helps

@omarahmed1111 omarahmed1111 requested a review from a team as a code owner February 10, 2025 13:41
@omarahmed1111 omarahmed1111 force-pushed the readd-lto-and-fix-the-pipeline-to-generate-ptx branch from 97248a9 to 554f8c6 Compare February 10, 2025 15:17
@asudarsa
Copy link
Contributor

asudarsa commented Feb 10, 2025

LGTM. Can we merge this?
Adding @sarnex for comment. cmake changes look good.

Thanks

@asudarsa asudarsa mentioned this pull request Feb 10, 2025
Closed
asudarsa
asudarsa approved these changes Feb 10, 2025
View reviewed changes
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

@omarahmed1111
Copy link
Contributor Author

omarahmed1111 commented Feb 12, 2025

@intel/llvm-reviewers-runtime Please review, Thanks!

uditagarwal97
uditagarwal97 approved these changes Feb 13, 2025
View reviewed changes
Copy link
Contributor

@uditagarwal97 uditagarwal97 left a comment

Choose a reason for hiding this comment

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

Changes in sycl/CMakeLists.txt LGTM

@omarahmed1111
Copy link
Contributor Author

omarahmed1111 commented Feb 14, 2025

@intel/llvm-gatekeepers Please merge, Thanks!

@kbenzie kbenzie merged commit fdea2cb into intel:sycl Feb 14, 2025
15 checks passed
@omarahmed1111 omarahmed1111 mentioned this pull request Feb 17, 2025
Merged
dm-vodopyanov pushed a commit that referenced this pull request Feb 21, 2025
[SYCL] Add clang-nvlink-wrapper to sycl-toolchain as a dependency (#1…
2ed0eba 
…7035)

This is a small follow-up PR on this
[PR](#16884) as `clang-nvlink-wrapper`
tool need to be added as a dependency for `sycl-toolchain`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AlexeySachkov AlexeySachkov AlexeySachkov approved these changes

@uditagarwal97 uditagarwal97 uditagarwal97 approved these changes

@mdtoguchi mdtoguchi mdtoguchi approved these changes

@asudarsa asudarsa asudarsa approved these changes

@sergey-semenov sergey-semenov Awaiting requested review from sergey-semenov sergey-semenov is a code owner automatically assigned from intel/llvm-reviewers-runtime

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@omarahmed1111 @AlexeySachkov @asudarsa @uditagarwal97 @mdtoguchi @kbenzie