Skip to content

[SYCL][CMAKE] Only apply extra warnings to SYCL RT #19306

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

Open
wants to merge 2 commits into
base: sycl
Choose a base branch
from

Conversation

AlexeySachkov
Copy link
Contributor

We can't enable all warnings globally, because LLVM itself isn't warning-free. Therefore, moved all necessary flags to the sycl subproject.

There are more places where we need those extra flags, but they will be added there in separate PRs because in absolute cases it requires addional fixes to the codebase.

We can't enable all warnings globally, because LLVM itself isn't
warning-free. Therefore, moved all necessary flags to the `sycl`
subproject.

There are more places where we need those extra flags, but they will be
added there in separate PRs because in absolute cases it requires
addional fixes to the codebase.
@AlexeySachkov AlexeySachkov marked this pull request as ready for review July 4, 2025 17:50
@AlexeySachkov AlexeySachkov requested review from a team as code owners July 4, 2025 17:50
@maarquitos14
Copy link
Contributor

We can't enable all warnings globally, because LLVM itself isn't warning-free.

I'm a bit confused. How was this working up until now then?

@AlexeySachkov
Copy link
Contributor Author

We can't enable all warnings globally, because LLVM itself isn't warning-free.

I'm a bit confused. How was this working up until now then?

AddSecurityFlags.cmake only kicks in if you pass --add_security_flags=default|sanitizer to configure.py. We don't have a CI configuration where we enable that option. We briefly had it enabled on one of our release branches, but immediately reverted because of issues that I'm trying to address in this and several other PRs.

Once this configuration is buildable, it will be added to nightly.

@maarquitos14
Copy link
Contributor

We can't enable all warnings globally, because LLVM itself isn't warning-free.

I'm a bit confused. How was this working up until now then?

AddSecurityFlags.cmake only kicks in if you pass --add_security_flags=default|sanitizer to configure.py. We don't have a CI configuration where we enable that option. We briefly had it enabled on one of our release branches, but immediately reverted because of issues that I'm trying to address in this and several other PRs.

Once this configuration is buildable, it will be added to nightly.

I see, thanks for the explanation!

@AlexeySachkov
Copy link
Contributor Author

First attempt at BMG E2E testing failed, but the test passed on restart. I expect this patch to be a non-functional change, so I will proceed with the merge.

Failed test:

Test d:\github\_work\llvm\llvm\build-e2e\reduction\output\reduction_ctor.cpp.tmp.out hung!

@KornevNikita
Copy link
Contributor

KornevNikita commented Jul 7, 2025

Hm, doesn't this do the same?

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra")

@AlexeySachkov
Copy link
Contributor Author

AlexeySachkov commented Jul 7, 2025

Hm, doesn't this do the same?

set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra")

Yeah, some cleanup is definitely due :)

UPD: I think is that the piece of code I touched should be outlined into some macro to be re-used somewhere else

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