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

[Driver][SYCL] Use existing option for device-only and some cleanup #15274

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

mdtoguchi
Copy link
Contributor

Update -fsycl-device-only usage to be an alias to --offload-device-only. This requires an additional unique check for SYCL enabling due to the expectation that -fsycl-device-only also implies -fsycl.

Update associated tests that use -fsycl-device-only -emit-llvm. Our default is already bitcode output, fixing this up aligns better with community expectations when using --offload-device-only.

Also do some variable cleanup to better be in sync with community variable names. And use a more common diagnostic for invalid options with -fsycl.

Update -fsycl-device-only usage to be an alias to --offload-device-only.
This requires an additional unique check for SYCL enabling due to the
expectation that -fsycl-device-only also implies -fsycl.

Update associated tests that use -fsycl-device-only -emit-llvm.  Our
default is already bitcode output, fixing this up aligns better with
community expectations when using --offload-device-only.

Also do some variable cleanup to better be in sync with community
variable names. And use a more common diagnostic for invalid options
with -fsycl.
@bader
Copy link
Contributor

bader commented Sep 3, 2024

Update associated tests that use -fsycl-device-only -emit-llvm. Our default is already bitcode output, fixing this up aligns better with community expectations when using --offload-device-only.

I find this behavior a bit confusing. My understanding is that by --offload-device-only should produce the device code in "executable" format as standard compiler invocation does for the host compilation.
SPIR target has a limitation. We don't have a native back-end for that target, but we can use SPIR-V translator as SPIR backend compiler to create a SPIR-V format. In that flow, using --offload-device-only --emit-llvm is reasonable.
This could be discussed and addressed separately.

@mdtoguchi
Copy link
Contributor Author

Update associated tests that use -fsycl-device-only -emit-llvm. Our default is already bitcode output, fixing this up aligns better with community expectations when using --offload-device-only.

I find this behavior a bit confusing. My understanding is that by --offload-device-only should produce the device code in "executable" format as standard compiler invocation does for the host compilation. SPIR target has a limitation. We don't have a native back-end for that target, but we can use SPIR-V translator as SPIR backend compiler to create a SPIR-V format. In that flow, using --offload-device-only --emit-llvm is reasonable. This could be discussed and addressed separately.

To support the ability to emit SPIR-V using the translator, we have -fsycl-device-only -fsycl-device-obj=spirv. Our default is -fsycl-device-obj=llvmir

@bader
Copy link
Contributor

bader commented Sep 3, 2024

To support the ability to emit SPIR-V using the translator, we have -fsycl-device-only -fsycl-device-obj=spirv. Our default is -fsycl-device-obj=llvmir

Right. The primary reason for that is to support Relocatable Device Code (-fgpu-rdc) by default. We keep device code in LLVM format to be able to link translation units with device code.

My point is that community compiler uses no-gpu-rdc by default (see https://clang.llvm.org/docs/HIPSupport.html#device-code-compilation). Potentially SPIR-V is also "linkable" file format, but we don't have a production quality SPIR-V linker tool.

We can also consider switching to no-gpu-rdc for SYCL mode by the default as well. This should be a separate discussion, so, please, do not consider my comments as blocking.

@mdtoguchi mdtoguchi marked this pull request as ready for review September 4, 2024 00:42
@mdtoguchi mdtoguchi requested review from a team as code owners September 4, 2024 00:42
@mdtoguchi
Copy link
Contributor Author

@intel/llvm-gatekeepers, this should be ready for merge, thanks!

@againull againull merged commit fd4b409 into intel:sycl Sep 4, 2024
13 checks passed
hvdijk added a commit to hvdijk/dpcppllvm that referenced this pull request Sep 9, 2024
…and some cleanup (intel#15274)"

This partially reverts commit fd4b409,
undoing the change to alias -fsycl-device-only and
--offload-device-only but keeping the other cleanups, and adds a test
showing why the alias does not work.

Fixes intel#15319
martygrant pushed a commit that referenced this pull request Sep 16, 2024
#15328)

…and some cleanup (#15274)"

This partially reverts commit fd4b409,
undoing the change to alias -fsycl-device-only and --offload-device-only
but keeping the other cleanups, and adds a test showing why the alias
does not work.

Fixes #15319
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.

7 participants