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][Driver] Enhance -Xarch_device and -Xarch_host for sycl #12478

Merged
merged 11 commits into from
Feb 5, 2024

Conversation

jinge90
Copy link
Contributor

@jinge90 jinge90 commented Jan 24, 2024

Add simple support to enhance -Xarch_device and -Xarch_host in sycl offloading which can support multiple arguments in single '-Xarch_*' and '-mllvm ' is supported as well.

@jinge90 jinge90 requested a review from a team as a code owner January 24, 2024 06:19
@jinge90 jinge90 marked this pull request as draft January 24, 2024 06:19
@jinge90
Copy link
Contributor Author

jinge90 commented Jan 24, 2024

Hi, @mdtoguchi and @AaronBallman
As we discussed before, this draft patch implements "--target-options=" and "--host-options=", could you help review whether the work is in correct direction?
Thanks very much.

@mdtoguchi
Copy link
Contributor

I encountered other options: -Xarch_host <arg> and -Xarch_device <arg> which can be used to push options to either the host or device compilation. Quick checking shows this works for various options, but does not work for any options that take arguments (like -mllvm <opt>). Would it make more sense to leverage that instead of introducing new options?

@jinge90
Copy link
Contributor Author

jinge90 commented Jan 25, 2024

-mllvm

Hi, @mdtoguchi
Nice finding! I think -Xarch_device and -Xarch_host have almost same behavior as --target-options and --host-options except flag such as '-mllvm ' and if users want to pass a series of flags, they need to type -Xarch_device or -Xarch_host multiple times.
I updated the patch to enhance current -Xarch_device and -Xarch_host, with it, users can pass a series of options via a single -Xarch_device/host like:
clang++ -fsycl test.cpp -Xarch_device "-fsanitize=address -DMY_DUMMY_MACRO -mllvm -enable-merge-functions ...."
This command will be translated to following internally:
clang++ -fsycl test.cpp -Xarch_device -fsanitize=address -Xarch_device -DMY_DUMMY_MACRO -Xarch -mllvm=-enable-merge-functions ....
As you can see '-mllvm ' will be converted to '-mllvm=' and passed to -Xarch_device or -Xarch_host.
Do you think we can proceed with this approach?
Thanks very much.

@jinge90 jinge90 changed the title [SYCL][Driver] Support --target-options/--host-options for SYCL device and host compilation [SYCL][Driver] Enhace -Xarch_device and -Xarch_host for sycl Jan 26, 2024
Copy link
Contributor

github-actions bot commented Jan 26, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@jinge90 jinge90 marked this pull request as ready for review February 2, 2024 08:52
Signed-off-by: jinge90 <[email protected]>
Signed-off-by: jinge90 <[email protected]>
@mdtoguchi mdtoguchi changed the title [SYCL][Driver] Enhace -Xarch_device and -Xarch_host for sycl [SYCL][Driver] Enhance -Xarch_device and -Xarch_host for sycl Feb 5, 2024
Copy link
Contributor

@mdtoguchi mdtoguchi left a comment

Choose a reason for hiding this comment

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

This looks OK to me. I did notice that currently this implementation does not apply to Windows. Moving forward, we should add Windows support.

@jinge90
Copy link
Contributor Author

jinge90 commented Feb 5, 2024

This looks OK to me. I did notice that currently this implementation does not apply to Windows. Moving forward, we should add Windows support.

Hi, @mdtoguchi
Thanks for pointing out this, I will create another PR to add tests for Windows target.
Thanks very much.

@jinge90 jinge90 requested a review from a team February 5, 2024 14:49
@jinge90
Copy link
Contributor Author

jinge90 commented Feb 5, 2024

Hi, @intel/llvm-gatekeepers
Could you help merge this patch?
Thanks very much.

@steffenlarsen steffenlarsen merged commit f7bdae8 into intel:sycl Feb 5, 2024
12 checks passed
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