[SYCL][NFC] Fix warnings coming out of SYCL headers. #3978

AlexeySachkov · 2021-06-23T07:44:32Z

The problem was discovered in #3777. Fixed by wrapping x
_SYCL_SPAN_ASSERT argument into parentheses.

Example of the warning/error:

sycl/CL/sycl/sycl_span.hpp:515:82: error: '&&' within '||' [-Werror,-Wlogical-op-parentheses]
    (static_cast <bool> ((_Count == dynamic_extent || _Count <= size() - _Offset && "Offset + count out of range in span::subspan()"))·
? void (0) : __assert_fail ("(_Count == dynamic_extent || _Count <= size() - _Offset && \"Offset + count out of range in span::subspan(
)\")", "/sycl/CL/sycl/sycl_span.hpp", 516, __extension__ __PRETTY_FUNCTION__));

sycl/CL/sycl/sycl_span.hpp:515:82: note: place parentheses around the '&&' expression· to silence this warning

The problem was discovered in /intel#3777. Fixed by wrapping `x` `_SYCL_SPAN_ASSERT` argument into parentheses. Example of the warning/error: ``` sycl/CL/sycl/sycl_span.hpp:515:82: error: '&&' within '||' [-Werror,-Wlogical-op-parentheses] (static_cast <bool> ((_Count == dynamic_extent || _Count <= size() - _Offset && "Offset + count out of range in span::subspan()"))· ? void (0) : __assert_fail ("(_Count == dynamic_extent || _Count <= size() - _Offset && \"Offset + count out of range in span::subspan( )\")", "/sycl/CL/sycl/sycl_span.hpp", 516, __extension__ __PRETTY_FUNCTION__)); sycl/CL/sycl/sycl_span.hpp:515:82: note: place parentheses around the '&&' expression· to silence this warning ```

dm-vodopyanov

LGTM. @bader, can you please merge it today when it will be convenient to unblock #3777?

bader · 2021-06-23T08:47:49Z

@AlexeySachkov, @dm-vodopyanov, I'm okay to merge this PR as soon as pre-commit tests pass, but ideally, the test case triggering the issue should be added to this PR.

AlexeySachkov · 2021-06-23T09:07:27Z

@AlexeySachkov, @dm-vodopyanov, I'm okay to merge this PR as soon as pre-commit tests pass, but ideally, the test case triggering the issue should be added to this PR.

The test which triggered the issue is existing warnings.cpp: logs, the issue is only reproducible when integration footer is involved. I will update the test

AlexeySachkov · 2021-06-23T09:08:23Z

I will update the test

Oops, too soon. The integration footer is not useable yet without #3777 and #3777 encounters this issue. I suggest that we merge this one as-is

bader · 2021-06-23T09:17:48Z

You could write a stand-alone reproducer calling _SYCL_SPAN_ASSERT with an expression passed to the first argument triggering the warning and tread it as an error. I don't think there should be dependency on integration footer or #3777.

AlexeySachkov · 2021-06-23T09:32:39Z

You could write a stand-alone reproducer calling _SYCL_SPAN_ASSERT with an expression passed to the first argument triggering the warning and tread it as an error. I don't think there should be dependency on integration footer or #3777.

I'm not exactly sure that this is a reliable thing, because I would expect _SYCL_SPAN_ASSERT to be undefined at the end of the file - it is not the case right now, but it is the case for a lot of internal marco. For example, the same problem with parentheses exists in types.hpp in __SYCL_GENERATE_CONVERT_IMPL macro - I won't be able to write isolated test for it, because the macro is undefined after its use.

Problem with types.hpp was found by manually launching the preprocessor first and then launching compiler on a preprocessed file - I don't know why we don't see those warnings if we go through a single-command compilation

bader · 2021-06-23T09:42:55Z

Problem with types.hpp was found by manually launching the preprocessor first and then launching compiler on a preprocessed file - I don't know why we don't see those warnings if we go through a single-command compilation

I assume it's because SYCL headers are handled as system/compiler headers...

bader · 2021-06-23T09:46:23Z

Did you try to build the test in non-SYCL mode (i.e. w/o -fsycl flag)?

AlexeySachkov · 2021-06-23T09:52:00Z

Did you try to build the test in non-SYCL mode (i.e. w/o -fsycl flag)?

Not yet, but it seems that I was able to reproduce the original problem by explicitly instantiating some SYCL classes in the test

bader · 2021-06-23T09:56:15Z

Jenkins/Precommit is still running, but I see failed tests already.

AlexeySachkov · 2021-06-23T09:56:40Z

Problem with types.hpp was found by manually launching the preprocessor first and then launching compiler on a preprocessed file - I don't know why we don't see those warnings if we go through a single-command compilation

I assume it's because SYCL headers are handled as system/compiler headers...

--no-system-header-prefix=CL/sycl should have prevented that, but seems like it doesn't work in all cases

AlexeySachkov · 2021-06-23T10:06:37Z

Did you try to build the test in non-SYCL mode (i.e. w/o -fsycl flag)?

@bader, I've tried this and it doesn't catch errors which are visible with manual preprocessing + compilation of preprocessed file steps

bader · 2021-06-23T10:13:57Z

Not yet, but it seems that I was able to reproduce the original problem by explicitly instantiating some SYCL classes in the test

I think the problem with reproducing the issue might be that the problem is in the template which is not instantiated. When the code is compiled, the both proposed ways should work (i.e. -fsycl + --no-system-header-prefix=CL/sycl and no -fsycl + -I/path/to/sycl/headers).

AlexeySachkov · 2021-06-23T10:23:20Z

Not yet, but it seems that I was able to reproduce the original problem by explicitly instantiating some SYCL classes in the test

I think the problem with reproducing the issue might be that the problem is in the template which is not instantiated. When the code is compiled, the both proposed ways should work (i.e. -fsycl + --no-system-header-prefix=CL/sycl and no -fsycl + -I/path/to/sycl/headers).

Still can't reproduce them with the following commands:

// RUN: %clangxx -fsycl --no-system-header-prefix=CL/sycl -fsyntax-only -Wall -Wextra -Wno-ignored-attributes -Wno-deprecated-declarations -Wpessimizing-move -Wunused-variable -Wmismatched-tags -Wunneeded-internal-declaration -Werror -Wno-unknown-cuda-version -Wno-unused-parameter %s -o %t.out                                                                                             
// RUN: %clangxx -std=c++17 -I %sycl_include -I %sycl_include/sycl -fsyntax-only -Wall -Wextra -Wno-ignored-attributes -Wno-deprecated-declarations -Wpessimizing-move -Wunused-variable -Wmismatched-tags -Wunneeded-internal-declaration -Werror -Wno-unknown-cuda-version -Wno-unused-parameter %s -o %t.out

sycl/test/warnings/warnings.cpp

bader · 2021-06-23T10:29:59Z

Not yet, but it seems that I was able to reproduce the original problem by explicitly instantiating some SYCL classes in the test

I think the problem with reproducing the issue might be that the problem is in the template which is not instantiated. When the code is compiled, the both proposed ways should work (i.e. -fsycl + --no-system-header-prefix=CL/sycl and no -fsycl + -I/path/to/sycl/headers).

Still can't reproduce them with the following commands:
// RUN: %clangxx -fsycl --no-system-header-prefix=CL/sycl -fsyntax-only -Wall -Wextra -Wno-ignored-attributes -Wno-deprecated-declarations -Wpessimizing-move -Wunused-variable -Wmismatched-tags -Wunneeded-internal-declaration -Werror -Wno-unknown-cuda-version -Wno-unused-parameter %s -o %t.out                                                                                             
// RUN: %clangxx -std=c++17 -I %sycl_include -I %sycl_include/sycl -fsyntax-only -Wall -Wextra -Wno-ignored-attributes -Wno-deprecated-declarations -Wpessimizing-move -Wunused-variable -Wmismatched-tags -Wunneeded-internal-declaration -Werror -Wno-unknown-cuda-version -Wno-unused-parameter %s -o %t.out                                                                                    

Interesting... can you dump the driver actions and compare with the actions from your reproducer?

AlexeySachkov · 2021-06-23T10:31:07Z

sycl/test/warnings/warnings.cpp

@@ -1,17 +1,42 @@
 // RUN: %clangxx -fsycl --no-system-header-prefix=CL/sycl -fsyntax-only -Wall -Wextra -Wno-ignored-attributes -Wno-deprecated-declarations -Wpessimizing-move -Wunused-variable -Wmismatched-tags -Wunneeded-internal-declaration -Werror -Wno-unknown-cuda-version -Wno-unused-parameter %s -o %t.out
-
+// RUN: %clangxx -fsycl -E --no-system-header-prefix=CL/sycl %s -o %t.ii
+// RUN: %clangxx -fsycl -fsyntax-only -Wall -Wextra -Wno-ignored-attributes -Wno-deprecated-declarations -Wpessimizing-move -Wunused-variable -Wmismatched-tags -Wunneeded-internal-declaration -Werror -Wno-unknown-cuda-version -Wno-unused-parameter %t.ii -o %t.out


BTW, I don't know why -Wno-unused-parameter was added here, but we can pass this test just fine without disabling that diagnostic

Outlined this into #3980

AlexeySachkov · 2021-06-23T10:34:24Z

Interesting... can you dump the driver actions and compare with the actions from your reproducer?

From what I see, both clang++ -fsycl and just clang++ do not launch the preprocessor as a separate step, i.e. there are no separate clang invocation with -E

* upstream/sycl: (489 commits) [SYCL][NFC] Lower overhead of making plugin calls (intel#3982) [SYCL][NFC] Use default macro initialization where applicable (intel#3979) [SYCL] Enable SPV_INTEL_fpga_invocation_pipelining_attributes extension (intel#3864) [SYCL] Disable reassociate pass to reduce register pressure (intel#3615) [Driver][SYCL][FPGA] Restrict -O0 for FPGA with hardware (intel#3966) [SYCL][NFC] Fix warnings coming out of SYCL headers. (intel#3978) [SYCL] Fix bugs with recursion in SYCL kernel (intel#3958) [SYCL][LevelZero] Add support to detect host->device and device->host transfers for USM (intel#3975) [SYCL] Enable native FP atomics by default (intel#3869) [sycl-post-link] Avoid copying from nullptr (intel#3963) [SYCL-PTX] Add warp-reduce path in sub-group reduce (intel#3949) [BuildBot] Uplift CPU/FPGAEMU RT version for CI Process (intel#3946) Fix handling of complex constant expressions Handle OpSpecConstantOp with CompositeExtract and CompositeInsert Handle OpSpecConstantOp with VectorShuffle [FuncSpec] Don't specialise functions with NoDuplicate instructions. [mlir][linalg] Support low padding in subtensor(pad_tensor) lowering [gn build] Port 208332d [AMDGPU] Add Optimize VGPR LiveRange Pass. [mlir][Linalg] NFC - Drop unused variable definition. ...

AlexeySachkov requested a review from a team as a code owner June 23, 2021 07:44

AlexeySachkov requested a review from cperkinsintel June 23, 2021 07:44

dm-vodopyanov previously approved these changes Jun 23, 2021

View reviewed changes

Fix one more warning and improve warnings LIT test

957502c

AlexeySachkov dismissed dm-vodopyanov’s stale review via 957502c June 23, 2021 10:10

AlexeySachkov requested a review from turinevgeny as a code owner June 23, 2021 10:10

bader changed the title ~~[SYCL][NFC] Fix warning coming out of sycl_span.hpp~~ [SYCL][NFC] Fix warnings coming out of SYCL headers. Jun 23, 2021

bader reviewed Jun 23, 2021

View reviewed changes

sycl/test/warnings/warnings.cpp Outdated Show resolved Hide resolved

Fix a typo

fcebde1

AlexeySachkov commented Jun 23, 2021

View reviewed changes

Apply comments

b16be58

bader approved these changes Jun 23, 2021

View reviewed changes

bader requested a review from dm-vodopyanov June 23, 2021 13:49

dm-vodopyanov approved these changes Jun 23, 2021

View reviewed changes

turinevgeny approved these changes Jun 23, 2021

View reviewed changes

bader merged commit 33d9af3 into intel:sycl Jun 23, 2021

AlexeySachkov mentioned this pull request Jun 23, 2021

Consider improving warnings.cpp LIT tests #3980

Open

AlexeySachkov deleted the private/asachkov/fix-sycl-span-warning branch May 22, 2024 09:49

[SYCL][NFC] Fix warnings coming out of SYCL headers. #3978

[SYCL][NFC] Fix warnings coming out of SYCL headers. #3978

Uh oh!

Conversation

AlexeySachkov commented Jun 23, 2021

Uh oh!

dm-vodopyanov left a comment

Choose a reason for hiding this comment

Uh oh!

bader commented Jun 23, 2021

Uh oh!

AlexeySachkov commented Jun 23, 2021

Uh oh!

AlexeySachkov commented Jun 23, 2021

Uh oh!

bader commented Jun 23, 2021

Uh oh!

AlexeySachkov commented Jun 23, 2021

Uh oh!

bader commented Jun 23, 2021

Uh oh!

bader commented Jun 23, 2021

Uh oh!

AlexeySachkov commented Jun 23, 2021

Uh oh!

bader commented Jun 23, 2021

Uh oh!

AlexeySachkov commented Jun 23, 2021

Uh oh!

AlexeySachkov commented Jun 23, 2021

Uh oh!

bader commented Jun 23, 2021

Uh oh!

AlexeySachkov commented Jun 23, 2021

Uh oh!

Uh oh!

bader commented Jun 23, 2021

Uh oh!

AlexeySachkov Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov Jun 23, 2021

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov commented Jun 23, 2021

Uh oh!

Uh oh!