Skip to content

Make ICX enabled again #1030

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

Merged
merged 3 commits into from
Jan 10, 2025
Merged

Make ICX enabled again #1030

merged 3 commits into from
Jan 10, 2025

Conversation

KFilipek
Copy link
Contributor

@KFilipek KFilipek commented Jan 9, 2025

Description

There is a problem with linkage under icx/icpx due to linking against compiler libs.
When executing tests without sourced paths to compiler (prevents from loading compiler version of UMF) there are errors of missing symbols/libs.

Considerations

There was 2 options:

  • link statically against compiler dependencies using -static-intel
  • omit linking Intel compiler extensions with -no-intel-lib (more info) or using another option -Wl,-as-needed'

First works like a charm, but the UMF contains icx/icpx needed dependencies compiled in, I don't know it's a problem or not.
Last option require to force gtest to compile the same way - without Intel compiler dependencies (e.g. svml, imf etc). According to gtest documentation we can share CMAKE_(C/CXX)_FLAGS. Another approach is to split steps and apply patch.

Fixes #843

Checklist

  • Code compiles without errors locally
  • All tests pass locally
  • CI workflows execute properly
  • CI workflows, not executed per PR (e.g. Nightly), execute properly
  • New tests added, especially if they will fail without my changes
  • Added/extended example(s) to cover this functionality
  • Extended the README/documentation
  • All newly added source files have a license
  • All newly added source files are referenced in CMake files
  • Logger (with debug/info/... messages) is used

@KFilipek KFilipek self-assigned this Jan 9, 2025
@KFilipek KFilipek force-pushed the test-icx branch 2 times, most recently from 6ed3b8e to c480959 Compare January 9, 2025 13:50
@lplewa lplewa changed the title Enable ICX again Make ICX enabled again Jan 9, 2025
@KFilipek KFilipek marked this pull request as ready for review January 9, 2025 15:11
@KFilipek KFilipek requested a review from a team as a code owner January 9, 2025 15:11
@KFilipek KFilipek requested review from ldorau and PatKamin January 10, 2025 10:03
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.

heh, I appreciate using my branch for this PR, but it contains a test commit "[CI] test only icx" - I guess it should be removed ;-)

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.

beside copyright and removing redundant commit - LGTM

@KFilipek
Copy link
Contributor Author

heh, I appreciate using my branch for this PR, but it contains a test commit "[CI] test only icx" - I guess it should be removed ;-)

I left it intentionally because it was also your work :P

@lukaszstolarczuk lukaszstolarczuk merged commit 9d98b90 into oneapi-src:main Jan 10, 2025
78 checks passed
@KFilipek KFilipek deleted the test-icx branch January 10, 2025 13:45
lukaszstolarczuk added a commit to lukaszstolarczuk/unified-memory-framework that referenced this pull request Feb 7, 2025
@lukaszstolarczuk lukaszstolarczuk mentioned this pull request Feb 7, 2025
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.

Re-enable ICX test in CI
5 participants