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

[SYCL] Creating version-agnostic copy of the SYCL import library #12889

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

againull
Copy link
Contributor

@againull againull commented Mar 1, 2024

Reason why this is needed is because some users want to explicitly link with sycl library and they don't want to update version all the time.

It is possible to generate only version-agnostic .lib files but I think we need to keep versioned files as well in case if somebody will have a scenario when multiple releases are in the LIB env variable and they want to link with specific version explicitly.

Ultimate goal is to get rid of most of the usages SYCL_MAJOR_VERSION in
clang, so that we don't need to keep that version in sync with SYCL RT.
Currently only one usage left which is related to windows-gnu triple which
is something we don't test (using compiler in mingw environment) and that
will need to be handled separately, as I wasn't able to succesfully
build/experiment with that.

Other reason why this is needed is because some users want to
explicitely link with sycl library and they don't want to update version
all the time.

It is possible to just generated version-agnostic .lib files but I
think we need to keep versioned files as well in case if somebody will
have a scenario when multiple releases are in the LIB env variable and
they want to link with specific version explicitely.
@againull
Copy link
Contributor Author

againull commented Mar 4, 2024

@sergey-semenov Could you please take a look.

else
CmdArgs.push_back("--dependent-lib=sycl" SYCL_MAJOR_VERSION "d");
CmdArgs.push_back("--dependent-lib=sycld");
Copy link
Contributor

Choose a reason for hiding this comment

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

As the versions progress - is there any way of verifying that the proper version of the library is being picked up in case there is some overlap in environment when resolving the libraries?

Copy link
Contributor Author

@againull againull Mar 5, 2024

Choose a reason for hiding this comment

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

I was sure that it is guaranteed that we take correct .lib file from "lib" directory of compiler installation because clang driver adds /libpath option with full path like this:
/libpath:[path to lib directory] /defaultlib:sycl.lib

From the description of the libpath option:
https://learn.microsoft.com/en-us/cpp/build/reference/libpath-additional-libpath?view=msvc-170
"Use the /LIBPATH option to override the environment library path. The linker will first search in the path specified by this option, and then search in the path specified in the LIB environment variable."

It indeed works as described. I.e. even if there are multiple installations of different sycl compilers and LIB env variable is set to point to all of them, we are going to link with proper .lib file even if it doesn't have version in the name. More over, for example, we link with msvcrt lib which doesn't have version as well.

So, I was trying to create additional test for this by your request.
But while experimenting I realized that there is a special case: current directory.
For example, if there is a file named sycl.lib (or sycl7.lib) in the current directory, then it is used at linkage regardless of libpath option or LIB env variable.
It is possible to avoid that issue if to use full path to .lib libraries like this: 862994c

I don't know a why msvc linker behaves that way for current directory and if this case is worth being worried about.

Since original intent of these changes wasn't to get rid of version from clang driver, I am going to revert that part of the changes in this PR. I can add them in further PR if we consider worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @againull - that sounds like a good plan.

@againull
Copy link
Contributor Author

againull commented Mar 6, 2024

Flaky failure on Windows Gen12:


Failed Tests (1):
SYCL :: ESIMD/dword_local_accessor_atomic_smoke.cpp

is unrelated. Disabled here: #12914
and reported in internal tracker.

@againull againull merged commit 2d2e418 into intel:sycl Mar 6, 2024
11 of 12 checks passed
@againull againull deleted the symlink_sycl branch April 9, 2024 19:56
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.

3 participants