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

Wrap linker flags on Windows for IntelLLLVM #2179

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

Maetveis
Copy link
Contributor

@Maetveis Maetveis commented Oct 8, 2024

The Intel C++ compiler requires linker flags to be wrapped, because CMake passes them through the compiler driver.
Prefix linker options with LINKER:.
CMake will transorm it to the appropriate flag for the compiler driver: (Nothing for MSVC or clang-cl, and /Qoption,link for icx), so this will work for other compilers and for earlier CMake versions too.

Note

Requires updating the fetched versions of UMF and Level Zero to versions that include the fix for the same problem. I tested locally with -DUMF_TAG=e6ff45e1636bd172117bab6d9f4d00638113f592 -DUR_LEVEL_ZERO_LOADER_TAG=v1.18.1.

Fixes: #2178

@Maetveis Maetveis marked this pull request as ready for review October 8, 2024 07:20
@Maetveis Maetveis requested review from a team as code owners October 8, 2024 07:20
@Maetveis
Copy link
Contributor Author

Maetveis commented Oct 8, 2024

Copy link
Contributor

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

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

👍 LGTM

FYI, similar PR was already verified and merged in umf: oneapi-src/unified-memory-framework#772

@lukaszstolarczuk
Copy link
Contributor

I can try and add an icx build in the CI, if you want...? (I'm doing such workflow for UMF at the moment)

@Maetveis
Copy link
Contributor Author

Ping, can we get this merged? I don't have merge access myself.

@kbenzie
Copy link
Contributor

kbenzie commented Oct 18, 2024

Ping, can we get this merged? I don't have merge access myself.

@oneapi-src/unified-runtime-level-zero-write still need to approve this.

@Maetveis
Copy link
Contributor Author

@kbenzie can you approve the workflows? I rebased to see if the CI failures go away or if I will need to investigate.

@jsji
Copy link
Contributor

jsji commented Oct 23, 2024

Ping @oneapi-src/unified-runtime-level-zero-write

@kbenzie
Copy link
Contributor

kbenzie commented Oct 23, 2024

@kbenzie can you approve the workflows? I rebased to see if the CI failures go away or if I will need to investigate.

The pull request labeler job is expected to fail for PRs from people who aren't a memory of the UR traige team. If there are e2e failures that doesn't necessarily block merging as long as we can determine they are not related to these changes.

@kbenzie kbenzie added loader Loader related feature/bug level-zero L0 adapter specific issues ready to merge Added to PR's which are ready to merge and removed ready to merge Added to PR's which are ready to merge labels Oct 25, 2024
Copy link
Contributor

@nrspruit nrspruit left a comment

Choose a reason for hiding this comment

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

LGTM for L0

@Maetveis
Copy link
Contributor Author

@nrspruit @kbenzie Can this get merged? I'm fairly certain the CI failures are not caused by this change.

@aarongreig
Copy link
Contributor

just going to rebase to see if we can get a clean run of the e2e l0 job

@Maetveis
Copy link
Contributor Author

Maetveis commented Nov 5, 2024

@aarongreig bump, should I look into the failures? I am not familiar with L0 and I don't expect runtime failures from this. This blocking some other future work for me.

@pbalcer
Copy link
Contributor

pbalcer commented Nov 5, 2024

No, those failures are expected.

@Maetveis
Copy link
Contributor Author

Maetveis commented Nov 5, 2024

No, those failures are expected.

Okay, Thank you. Lets get this merged then pretty please :).

@callumfare callumfare added the ready to merge Added to PR's which are ready to merge label Nov 5, 2024
@Maetveis
Copy link
Contributor Author

Maetveis commented Nov 8, 2024

ping @callumfare @nrspruit @pbalcer

@callumfare
Copy link
Contributor

@Maetveis Please resolve the conflicts and I'll make sure this gets merged soon

@Maetveis
Copy link
Contributor Author

Maetveis commented Nov 8, 2024

@Maetveis Please resolve the conflicts and I'll make sure this gets merged soon

Thanks! Done.

The Intel C++ compiler requires linker flags to be wrapped, because
CMake passes them through the compiler driver.
Prefix linker options with `LINKER:`.
CMake will transorm it to the appropriate flag for the compiler driver:
(Nothing for MSVC or clang-cl, and /Qoption,link for icx), so this will
work for other compilers and for earlier CMake versions too.

Signed-off-by: Gergely Meszaros <[email protected]>
@Maetveis
Copy link
Contributor Author

Since #2100 was reverted and it had a CMake fix that was needed here (the source of the conflict previously) I restored this PR to how it was before conflict resolution.

Ping @callumfare, please merge this ASAP.

@callumfare callumfare merged commit 9a209aa into oneapi-src:main Nov 12, 2024
79 of 83 checks passed
steffenlarsen pushed a commit to intel/llvm that referenced this pull request Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
level-zero L0 adapter specific issues loader Loader related feature/bug 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.

Unescaped linker flags when building with the Intel C++ Compiler on Windows and CMake > 3.25
9 participants