Skip to content

[SYCL][Driver] Emit unused argument warning for -fno-libspirv #19135

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 2, 2025
Merged

[SYCL][Driver] Emit unused argument warning for -fno-libspirv #19135

merged 3 commits into from
Jul 2, 2025

Conversation

Maetveis
Copy link
Contributor

@Maetveis Maetveis commented Jun 25, 2025 •
edited
Loading

Based on PR #19134

Chain of upstream PRs & tree of downstream PRs as of 2025-06-25

Previously the -fno-libspirv option was not warned about when there
was no SYCL compilation.
Remove the explicit target check for the targets that support
-fno-libspirv and instead rely on each target to emit appropriate
warnings when its used.
This commit slightly degrades diagnostic quality, from

"ignoring '-fno-sycl-libspirv' option as it is not currently supported for target"
to
"argument unused during compilation: '-fno-sycl-libspirv'"
, but I believe this is acceptable as it allows to remove the list
of targets that support the option from the driver. Additionally,
now if the user mixes targets that support and do not support
-fno-libspirv in the same compilation, they will not get warnings
that are not actionable.

@Maetveis Maetveis self-assigned this Jun 25, 2025
This was referenced Jun 25, 2025
Draft
Merged
Merged
Merged
@Maetveis Maetveis force-pushed the review/Maetveis/libspirv-path-install-dir branch from 717300b to df0bc40 Compare June 25, 2025 09:01
@Maetveis Maetveis force-pushed the review/Maetveis/libspirv-unused-argument branch from eb713e6 to 8badf3c Compare June 25, 2025 09:02
@Maetveis Maetveis marked this pull request as ready for review June 25, 2025 09:14
@Maetveis Maetveis requested a review from a team as a code owner June 25, 2025 09:14
@Maetveis Maetveis force-pushed the review/Maetveis/libspirv-path-install-dir branch from df0bc40 to 491016c Compare June 26, 2025 15:58
@Maetveis Maetveis force-pushed the review/Maetveis/libspirv-unused-argument branch from 8badf3c to db2c9d8 Compare June 26, 2025 15:59
@Maetveis Maetveis force-pushed the review/Maetveis/libspirv-path-install-dir branch from 491016c to c1b5a53 Compare June 27, 2025 07:22
@Maetveis Maetveis force-pushed the review/Maetveis/libspirv-unused-argument branch from db2c9d8 to eac8168 Compare June 27, 2025 07:23
steffenlarsen pushed a commit that referenced this pull request Jun 27, 2025
@Maetveis
[SYCL][Driver] Improve SYCL libspirv linking test coverage (NFC) (#19130
345b9ab 
)

<!-- start git-machete generated -->

# Based on PR #18956

## Tree of downstream PRs as of 2025-06-25

* **PR #19130 (THIS ONE)**

    * PR #19131

      * PR #19134

        * PR #19135

          * PR #19136

<!-- end git-machete generated -->

In the next commits I'd like to refactor and fix SYCL libspirv linking.
This adds a few tests to cover the current behavior. Some of it is
buggy, and not consistent between NVPTX and AMDGPU, it will be improved
in the next commits.
@Maetveis Maetveis force-pushed the review/Maetveis/libspirv-unused-argument branch from eac8168 to 11e238c Compare June 27, 2025 07:31
@Maetveis Maetveis force-pushed the review/Maetveis/libspirv-path-install-dir branch from c1b5a53 to 63704bf Compare June 27, 2025 07:37
@Maetveis Maetveis force-pushed the review/Maetveis/libspirv-unused-argument branch from 11e238c to d5440ef Compare June 27, 2025 07:37
@Maetveis Maetveis force-pushed the review/Maetveis/libspirv-path-install-dir branch from 63704bf to c0793c1 Compare June 27, 2025 08:33
@Maetveis Maetveis force-pushed the review/Maetveis/libspirv-unused-argument branch from d5440ef to e5c3890 Compare June 27, 2025 08:33
bader pushed a commit that referenced this pull request Jun 27, 2025
@Maetveis
[SYCL][Driver] Deduplicate libspirv logic (#19131)
7054619 
<!-- start git-machete generated -->

# Based on PR #19130

## Chain of upstream PRs & tree of downstream PRs as of 2025-06-25

* PR #19130

  * **PR #19131 (THIS ONE)**

      * PR #19134

        * PR #19135

          * PR #19136

<!-- end git-machete generated -->

Move the logic for finding and linking libspirv into
SYCLInstallationDetector.
This code was basically duplicated between the CUDA and HIP toolchains,
and was also present in the Driver sources.
This is NFC, aside from the fact that the code in the HIP toolchain
lacked the special handling for the `-###` driver option present in the
other two places.
Maetveis added 2 commits June 27, 2025 23:45
@Maetveis
[SYCL][Driver] Base libspirv path on sycl install candidates
3339a77 
... instead of the resource directory. This matches the behaviour of
SYCL device libraries. The resource directory has nothing to do with
libspirv, and it might be changed by the `-resource-dir` option, which
would lead us to not find it.
@Maetveis
[SYCL][Driver] Emit unused argument warning for -fno-libspirv
0bb5db5 
Previously the `-fno-libspirv` option was not warned about when there
was no SYCL compilation.
Remove the explicit target check for the targets that support
`-fno-libspirv` and instead rely on each target to emit appropriate
warnings when its used.
This commit slightly degrades diagnostic quality, from

"ignoring '-fno-sycl-libspirv' option as it is not currently supported for target"
to
"argument unused during compilation: '-fno-sycl-libspirv'"
, but I believe this is acceptable as it allows to remove the list
of targets that support the option from the driver. Additionally,
now if the user mixes targets that support and do not support
`-fno-libspirv` in the same compilation, they will not get warnings
that are not actionable.
@Maetveis Maetveis force-pushed the review/Maetveis/libspirv-path-install-dir branch from c0793c1 to 3339a77 Compare June 28, 2025 06:47
@Maetveis Maetveis force-pushed the review/Maetveis/libspirv-unused-argument branch from e5c3890 to 0bb5db5 Compare June 28, 2025 06:47
bader pushed a commit that referenced this pull request Jul 1, 2025
@Maetveis
[SYCL][Driver] Base libspirv path on sycl install candidates (#19134)
bf21b9d 
<!-- start git-machete generated -->

# Based on PR #19131

## Chain of upstream PRs & tree of downstream PRs as of 2025-06-25

* PR #19130

  * PR #19131

    * **PR #19134 (THIS ONE)**

        * PR #19135

          * PR #19136

<!-- end git-machete generated -->

... instead of the resource directory. This matches the behaviour of
SYCL device libraries. The resource directory has nothing to do with
libspirv, and it might be changed by the `-resource-dir` option, which
would lead us to not find it.
Base automatically changed from review/Maetveis/libspirv-path-install-dir to sycl July 1, 2025 02:14
@Maetveis
Copy link
Contributor Author

Maetveis commented Jul 1, 2025

@intel/llvm-gatekeepers this should be ready to merge.

aelovikov-intel
aelovikov-intel previously requested changes Jul 1, 2025
View reviewed changes
Copy link
Contributor

@aelovikov-intel aelovikov-intel left a comment

Choose a reason for hiding this comment

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

What did you do to skip CI completely here?

@sommerlukas
Copy link
Contributor

What did you do to skip CI completely here?

Could be because the base/target branch for this PR was initially not the sycl branch and only changed recently:

image

@Maetveis
Copy link
Contributor Author

Maetveis commented Jul 1, 2025 •
edited
Loading

What did you do to skip CI completely here?

Oops sorry, that was not intentional.

Could be because the base/target branch for this PR was initially not the sycl branch and only changed recently:

image

Yes, that's it. CI only runs on the sycl branch, but base branch change does not trigger a workflow. My usual workaround is to close and re-open, which does. Doing that now.

@Maetveis Maetveis closed this Jul 1, 2025
@Maetveis Maetveis reopened this Jul 1, 2025
@Maetveis Maetveis temporarily deployed to WindowsCILock July 1, 2025 15:01 — with GitHub Actions Inactive
@bader
Copy link
Contributor

bader commented Jul 1, 2025

base branch change does not trigger a workflow

@aelovikov-intel, this sounds like something we need to fix in the GHA workflow.

@aelovikov-intel
Copy link
Contributor

base branch change does not trigger a workflow

@aelovikov-intel, this sounds like something we need to fix in the GHA workflow.

Patches welcome :)

My usual workaround is to close and re-open, which does. Doing that now.

What I do is go to the source branch and "sync" it to merge latest destination branch to it. In most cases it's a non-noop merge and CI restarts.

@aelovikov-intel aelovikov-intel dismissed their stale review July 1, 2025 15:09

CI was re-triggered.

@Maetveis
Copy link
Contributor Author

Maetveis commented Jul 1, 2025

Patches welcome :)

FYI:

By default, a workflow only runs when a pull_request event's activity type is opened, synchronize, or reopened. To trigger workflows by different activity types, use the types keyword.

(https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request)

and https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#onevent_nametypes

IIRC base branch change was under the type "edited".

@Maetveis Maetveis temporarily deployed to WindowsCILock July 1, 2025 16:05 — with GitHub Actions Inactive
@Maetveis Maetveis temporarily deployed to WindowsCILock July 1, 2025 16:05 — with GitHub Actions Inactive
bader
bader reviewed Jul 1, 2025
View reviewed changes
@bader bader temporarily deployed to WindowsCILock July 1, 2025 17:11 — with GitHub Actions Inactive
@bader bader temporarily deployed to WindowsCILock July 1, 2025 18:25 — with GitHub Actions Inactive
@bader bader temporarily deployed to WindowsCILock July 1, 2025 18:25 — with GitHub Actions Inactive
@Maetveis
Copy link
Contributor Author

Maetveis commented Jul 2, 2025 •
edited
Loading

Thanks Alexey for fixing clang-format! @intel/llvm-gatekeepers this is ready to merge now.

@steffenlarsen steffenlarsen merged commit 56800d0 into sycl Jul 2, 2025
25 checks passed
@Maetveis Maetveis deleted the review/Maetveis/libspirv-unused-argument branch July 2, 2025 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bader bader bader left review comments

@mdtoguchi mdtoguchi mdtoguchi approved these changes

@aelovikov-intel aelovikov-intel aelovikov-intel left review comments

Assignees

@Maetveis Maetveis

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Maetveis @sommerlukas @bader @aelovikov-intel @mdtoguchi @steffenlarsen