Skip to content

[SYCL] Fix compilation issue introduced by d0d8a56 #16264

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 1 commit into from
Dec 4, 2024

Conversation

Naghasan
Copy link
Contributor

@Naghasan Naghasan commented Dec 4, 2024

fix compilation issue introduced by #15061

fix compilation issue introduced by intel#15061
@Naghasan Naghasan requested a review from a team as a code owner December 4, 2024 16:18
@Naghasan Naghasan requested a review from maarquitos14 December 4, 2024 16:18
@Naghasan
Copy link
Contributor Author

Naghasan commented Dec 4, 2024

@sarnex the fix

@bader bader changed the title Fix postcommit failure [SYCL] Fix compilation issue introduced by d0d8a56 Dec 4, 2024
@sarnex
Copy link
Contributor

sarnex commented Dec 4, 2024

thx will merge once build passes

@sarnex
Copy link
Contributor

sarnex commented Dec 4, 2024

build passed, merging early because change is trivial and build is broken on multiple platforms. will monitor postcommit.

@sarnex sarnex merged commit 9e31e6a into intel:sycl Dec 4, 2024
13 of 14 checks passed
@Naghasan
Copy link
Contributor Author

Naghasan commented Dec 4, 2024

the error is caused by Werror, shouldn't that be used as well in the precommit build ? I understand why there is post commit tests, but I feel my failure could have been caught before.

@sarnex
Copy link
Contributor

sarnex commented Dec 4, 2024

it is used in precommit, we build both with -DSYCL_ENABLE_WERROR=ON

the problem is the compiler is different

precommit

2024-12-04T11:58:58.5567290Z   cc: gcc
2024-12-04T11:58:58.5567773Z   cxx: g++
2024-12-04T11:58:58.5568315Z   build_image: ghcr.io/intel/llvm/ubuntu2204_build:latest
2024-12-04T11:58:58.5568921Z   build_ref: 4b3842901ccba38a6a268fdafb4bfaf2dbf625fe

postcommit

2024-12-04T14:25:56.9074558Z   cc: clang
2024-12-04T14:25:56.9074850Z   cxx: clang++
2024-12-04T14:25:56.9075230Z   build_image: ghcr.io/intel/llvm/sycl_ubuntu2204_nightly:build

so it seems like gcc's werror is much more lenient. maybe we should also use clang in precommit, and add a nightly job to build in gcc just to make sure it works? opinions @aelovikov-intel @uditagarwal97

@sarnex
Copy link
Contributor

sarnex commented Dec 4, 2024

@Naghasan Build is still broken with seemingly the same error

2024-12-04T16:30:59.4365706Z /__w/llvm/llvm/src/sycl/source/detail/scheduler/commands.cpp:2512:56: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
2024-12-04T16:30:59.4366308Z  2512 |         {UR_EXP_LAUNCH_PROPERTY_ID_WORK_GROUP_MEMORY, {WorkGroupMemorySize}});

https://github.com/intel/llvm/actions/runs/12164078580/job/33924842681

Can you build with clang when verifying your fix locally?

@aelovikov-intel
Copy link
Contributor

maybe we should also use clang in precommit, and add a nightly job to build in gcc just to make sure it works? opinions @aelovikov-intel @uditagarwal97

I think the cases like this one are rare enough and the current split between pre/post does the right job. Also, why would anyone use gcc for their normal day-to-day work anyway? ;)

@sarnex
Copy link
Contributor

sarnex commented Dec 4, 2024

should we switch it so we use clang in precommit and gcc in post?

@uditagarwal97
Copy link
Contributor

should we switch it so we use clang in precommit and gcc in post?

If clang's werror is stricter, it makes sense to use clang in pre-commit and gcc in post.

@sarnex
Copy link
Contributor

sarnex commented Dec 4, 2024

ive never seen a werror issue in precommit, so that seems to be the case. ill try switching it.

@aelovikov-intel
Copy link
Contributor

I don't think is strictly "stricter" than the other. Wouldn't be surprised if we'll have same kind of situation after the switch.

Anyhow, up to @sarnex and @uditagarwal97 to decide as they're the ones keeping our CI running (and many thanks to them for that!)

@sarnex
Copy link
Contributor

sarnex commented Dec 4, 2024

thanks, ill try it and monitor it and if we see more postcommit fails the other way ill switch it back

@aelovikov-intel
Copy link
Contributor

While on it, do you think we can add a windows build with OneAPI release (vs MSVC) in post or nightly?

@sarnex
Copy link
Contributor

sarnex commented Dec 4, 2024

yeah let me try (gonna regret this prolly)

@sarnex
Copy link
Contributor

sarnex commented Dec 4, 2024

im fixing the build in #16268

@Naghasan
Copy link
Contributor Author

Naghasan commented Dec 4, 2024

thanks @sarnex , apologies I was trapped in a meeting

@sarnex
Copy link
Contributor

sarnex commented Dec 4, 2024

np

@sarnex
Copy link
Contributor

sarnex commented Dec 4, 2024

These kind of issues should be caught in precommit now because of 5e0db3e

AlexeySachkov pushed a commit to AlexeySachkov/llvm that referenced this pull request Mar 25, 2025
AlexeySachkov added a commit that referenced this pull request Mar 25, 2025
This is a cherry-pick of #16264 and #16268


Patch-by: Victor Lomuller <[email protected]>
Co-authored-by: Nick Sarnie <[email protected]>
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.

5 participants