[SYCL] Refactor LIT tests for diagnostics #3956

AlexeySachkov · 2021-06-18T17:18:16Z

Switched ESIMD diagnostic tests to use FileCheck instead of -verify.
Added %fsycl-host-only substitution, which contains a minimal amount
of compiler arguments needed to only perform SYCL source compilation in
host mode. Updated SYCL LIT tests checking diagnostics for host APIs to
use that new substitution.

This patch is motivated by a few of things:

enabling integration footer unconditionally in [SYCL] Switch to using integration footer by default #3777
the fact that -verify is intended to be used with clang -cc1
the fact that VerifyDiagnosticConsumer doesn't handle (or maybe
not even designed to) line markers in input file

All those things lead to LIT failures, because -verify is not able to
properly read diagnostic messages emitted by FE invocation for host part
of DPC++/SYCL programs and reports that some diagnostics are either
"seen, but not expected" or "expected, but not seen". Note: diagnostic
messages themself look just fine and correct when observed through
clang -fsycl, clang -cc1 -fsycl-device-only and clang -cc1 -fsycl-is-host invocations.

Added `-fsycl-device-only` to ESIMD-specific tests. Added `%fsycl-host-only` substitution, which contains a minimal amount of compiler arguments needed to only perform SYCL source compilation in host mode. Updated SYCL LIT tests checking diagnostics for host APIs to use that new substitution. This patch is motivated by a few of things: - enabling integration footer unconditionally in intel#3777 - the fact that `-verify` is intended to be used with `clang -cc1` - the fact that `VerifyDiagnosticConsumer` doesn't handle (or maybe not even designed to) line markers in input file All those things lead to LIT failures, because `-verify` is not able to properly read diagnostic messages emitted by FE invocation for host part of DPC++/SYCL programs and reports that some diagnostics are either "seen, but not expected" or "expected, but not seen". Note: diagnostic messages themself look just fine and correct when observed through `clang -fsycl`, `clang -cc1 -fsycl-device-only` and `clang -cc1 -fsycl-is-host` invocations. Besides resolving/avoiding those issues, this PR should speed up those LIT tests, because of reduced amount of FE invocations. Change to ESIMD-specific tests is debatable, because we now do not check diagnostics coming from the host compilation pass. Hopefully, this is not a huge problem, because: - ESIMD APIs don't seem to be usable on host - device compilation is a first phase in our toolchain so, diagnostics in shared code should still be tested with this change - it is expected for API endpoints to have the same requirements/arguments/return types/deprecation notices between host and device, which means that tests for those things can only be performed for either device or host and duplication could be avoided

AlexeySachkov · 2021-06-18T17:20:43Z

sycl/test/esimd/enums.cpp

@@ -1,4 +1,4 @@
-// RUN: %clangxx -fsycl -fsyntax-only -Xclang -verify %s
+// RUN: %clangxx -fsycl -fsycl-device-only -fsyntax-only -Xclang -verify %s


Copying discussion from #3777 here:

@DenisBakhvalov:

We still want to check host compilation of ESIMD APIS.
I found some explanation here 7669de4:

The reason for the change is that "-verify" mode does not work correctly
with new integration footer approach, because it seems to ignore line
markers in the input file.

But can you please elaborate on it? To me it seems that we silently hide some problem in host compilation.

@AlexeySachkov:

But can you please elaborate on it?

Sure, I plan to outline those diagnostics tests changes into a separate PR with more detailed commit message - stay tuned :)

To me it seems that we silently hide some problem in host compilation.

Hopefully, we don't have different API endpoints for host and device APIs. BTW, is ESIMD classes like simd are expected to work in host part of the program?

@DenisBakhvalov:

Hopefully, we don't have different API endpoints for host and device APIs.

Actually, for ESIMD, we do :) . device APIs are lowered into GenX intrinsics, while for host we have an impl using simple loops. That's why we still want to check host compilation, because it's two different paths.

BTW, is ESIMD classes like simd are expected to work in host part of the program?

No, but they are expected to work in the device part of the program executed on the host.

@DenisBakhvalov

Actually, for ESIMD, we do :) . device APIs are lowered into GenX intrinsics, while for host we have an impl using simple loops. That's why we still want to check host compilation, because it's two different paths.

It is ok for them to have different implementation between host and device and this is true for like almost all device APIs in our implementation, but I was talking more about the interface itself: I would expect it to be the same between host and device, i.e. same arguments, same names, same return values, same deprecation rules, etc. If that is not the case, then I think that something is wrong with design of the extension.

Please also take a look at the commit message: there are couple of other points related to this too

I'm not sure I understand how different implementation and design of the extension is relevant here.
For ESIMD we need to test host compilation even if it does not run on host device. So please don't add -fsycl-device-only for ESIMD tests.
I wonder why this is not the case for SYCL - same applies. Host compiler and device compiler use different code to compile - e.g. due to SYCL_DEVICE__ONLY, so having code compiled by device compiler does not mean it can be compiled by the host compiler.

This particular test checks that we have a warning about a method (or something else) being deprecated, it does not check full compilation or execution. Do we really expect to have an API which is deprecated on host, but not deprecated on device or vice versa?

I've read description of those tests again and indeed they were intended to not only check deprecation messages, but rather check that we can compile particular code, in which case host compilation must also be checked.

Unfortunately, we won't be able to use -vefiry for that, but it should be possible to workaround that with FileCheck, I will prepare an update for this PR

My main point above is that compilation should be checked for both host and device compiler, whether it is SYCL or ESIMD. Do you agree with that?

Regarding this particular test - it does check compilation, and this is one of its purposes, as well as checking the deprecation note. I'm not sure what do you mean by "full".

My main point above is that compilation should be checked for both host and device compiler, whether it is SYCL or ESIMD. Do you agree with that?

I still think that it depends on the particular test. For example some tests which I've modified do not even have device code in them and if the only thing we care is some diagnostic messages, then we could probably optimize them by avoiding unnecessary compiler passes.

I'm not sure what do you mean by "full".

Something closer to E2E test: modified tests contain -fsyntax-only, which means that we stop right after FE, I'm not even sure that LLVM IR is generated by clang in that case.

For tests w/o device code - maybe. Even though this might not be reliable too as device compiler may use different includes and can fail parsing/syntax check.

We don't need to generate IR in such tests, just check that API compiles, as this is not E2E tests.

We don't need to generate IR in such tests, just check that API compiles, as this is not E2E tests.

unless, of course, the purpose of the test is to check FE CG. There are some cases like these.

I've updated the PR to enable host compilation back for ESIMD tests in 118ec38, but I had to use FileCheck, because -verify won't work for host part in foreseeable feature

kbobrovs · 2021-06-18T17:47:29Z

sycl/test/esimd/enums.cpp

@@ -1,4 +1,4 @@
-// RUN: %clangxx -fsycl -fsyntax-only -Xclang -verify %s
+// RUN: %clangxx -fsycl -fsycl-device-only -fsyntax-only -Xclang -verify %s


I'm not sure I understand how different implementation and design of the extension is relevant here.
For ESIMD we need to test host compilation even if it does not run on host device. So please don't add -fsycl-device-only for ESIMD tests.
I wonder why this is not the case for SYCL - same applies. Host compiler and device compiler use different code to compile - e.g. due to SYCL_DEVICE__ONLY, so having code compiled by device compiler does not mean it can be compiled by the host compiler.

vladimirlaz · 2021-06-21T05:04:21Z

sycl/test/warnings/sycl_2020_deprecations.cpp

@@ -1,7 +1,5 @@
-// RUN: %clangxx -fsycl -sycl-std=2020 -Xclang -verify -Xclang -verify-ignore-unexpected=note %s -o %t.out
-// RUN: %clangxx -fsycl -Xclang -verify -Xclang -verify-ignore-unexpected=note %s -o %t.out
-// RUN: %clangxx -fsycl -sycl-std=2017 -Werror %s -o %t.out


Could you please comment on why do you remove the last two run lines? They check that no warnings are generated if the build is done for SYCL standards prior to SYCL 2020.

That is my bad, missed -sycl-std flag there, returned them in 118ec38

They check that no warnings are generated if the build is done for SYCL standards prior to SYCL 2020.

returned them in 118ec38

I don't think that "returned" checks do what Vladimir said i.e. "check that no warnings are generated if the build is done for SYCL standards prior to SYCL 2020". Right?

sycl/test/basic_tests/image_accessor_types.cpp

Returned host compiler invocation to some tests, but in order to do so I had to switch them to use `FileCheck` instead of `-verify` as the latter won't work with host code anymore once we enable integration footer.

AlexeySachkov · 2021-06-22T12:11:57Z

It looks like Jenkins/Precommit has failed due to infrastructure issues

kbobrovs

I've updated the PR to enable host compilation back for ESIMD tests in 118ec38, but I had to use FileCheck, because -verify won't work for host part in foreseeable feature

Do I understand correctly that it is the new integration footer feature that makes these tests fail now, and this is the main motivation?

sycl/test/esimd/simd_copy_to_copy_from.cpp

sycl/test/esimd/enums.cpp

AlexeySachkov · 2021-06-22T17:15:14Z

I've updated the PR to enable host compilation back for ESIMD tests in 118ec38, but I had to use FileCheck, because -verify won't work for host part in foreseeable feature

Do I understand correctly that it is the new integration footer feature that makes these tests fail now, and this is the main motivation?

That is correct. I think that this is a significant change to our tests and therefore I would like it to be preserved as a separate commit in git history - that is why I outlined test changes into a separate PR

v-klochkov · 2021-06-24T05:10:21Z

sycl/test/lit.cfg.py

@@ -90,6 +90,8 @@
 config.substitutions.append( ('%llvm_build_lib_dir',  config.llvm_build_lib_dir ) )
 config.substitutions.append( ('%llvm_build_bin_dir',  config.llvm_build_bin_dir ) )

+config.substitutions.append( ('%fsycl-host-only', '-std=c++17 -Xclang -fsycl-is-host -isystem %s -isystem %s -isystem %s' % (config.sycl_include, config.opencl_include_dir, config.sycl_include + '/sycl/') ) )


I am not sure -std=c++17 needs to be part of fsycl-host-only, but that is probably Ok, if that is intentional and has meaning "using sycl switches assumes using c++17"

if that is intentional and has meaning "using sycl switches assumes using c++17"

That was intentional, we can't compile our headers in c++14 mode, unfortunately. It is probably a bug, but for now I think that it is ok to stick with c++17 explicitly

sycl/test/esimd/simd_subscript.cpp

AlexeySachkov · 2021-06-25T07:34:52Z

Jenkins/Precommit is failed on Windows due to infrastructure issue - it seems like build was interrupted by some reason

vladimirlaz · 2021-06-25T07:39:48Z

Jenkins/Precommit is failed on Windows due to infrastructure issue - it seems like build was interrupted by some reason

@tfzhu, could you please comments?

romanovvlad · 2021-06-25T10:37:35Z

The failed job was restarted and has passed.

AlexeySachkov requested review from DenisBakhvalov, kbobrovs and a team as code owners June 18, 2021 17:18

AlexeySachkov requested a review from v-klochkov June 18, 2021 17:18

AlexeySachkov commented Jun 18, 2021

View reviewed changes

kbobrovs suggested changes Jun 18, 2021

View reviewed changes

vladimirlaz suggested changes Jun 21, 2021

View reviewed changes

Apply review comments

118ec38

Returned host compiler invocation to some tests, but in order to do so I had to switch them to use `FileCheck` instead of `-verify` as the latter won't work with host code anymore once we enable integration footer.

AlexeySachkov requested review from kbobrovs and vladimirlaz June 22, 2021 12:11

kbobrovs reviewed Jun 22, 2021

View reviewed changes

sycl/test/esimd/simd_copy_to_copy_from.cpp Outdated Show resolved Hide resolved

sycl/test/esimd/enums.cpp Outdated Show resolved Hide resolved

Apply comments

0da8cdc

AlexeySachkov requested a review from kbobrovs June 23, 2021 07:07

kbobrovs previously approved these changes Jun 23, 2021

View reviewed changes

v-klochkov reviewed Jun 24, 2021

View reviewed changes

vladimirlaz previously approved these changes Jun 24, 2021

View reviewed changes

bader reviewed Jun 24, 2021

View reviewed changes

sycl/test/esimd/simd_subscript.cpp Outdated Show resolved Hide resolved

AlexeySachkov marked this pull request as draft June 24, 2021 12:45

AlexeySachkov added 2 commits June 24, 2021 16:49

temp

18158a4

Ensure that no unexpected diagnostics emitted with FileCheck

35b6ef9

AlexeySachkov dismissed stale reviews from vladimirlaz and kbobrovs via 35b6ef9 June 24, 2021 16:18

AlexeySachkov marked this pull request as ready for review June 24, 2021 16:19

kbobrovs approved these changes Jun 24, 2021

View reviewed changes

vladimirlaz approved these changes Jun 25, 2021

View reviewed changes

romanovvlad merged commit 8445dc1 into intel:sycl Jun 25, 2021

AlexeySachkov deleted the private/asachkov/refactor-sycl-verify-tests branch May 22, 2024 09:49

		@@ -1,4 +1,4 @@
		// RUN: %clangxx -fsycl -fsyntax-only -Xclang -verify %s
		// RUN: %clangxx -fsycl -fsycl-device-only -fsyntax-only -Xclang -verify %s

[SYCL] Refactor LIT tests for diagnostics #3956

[SYCL] Refactor LIT tests for diagnostics #3956

Uh oh!

Conversation

AlexeySachkov commented Jun 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexeySachkov commented Jun 22, 2021

Uh oh!

kbobrovs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AlexeySachkov commented Jun 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexeySachkov commented Jun 25, 2021

Uh oh!

vladimirlaz commented Jun 25, 2021

Uh oh!

romanovvlad commented Jun 25, 2021

Uh oh!

Uh oh!

AlexeySachkov commented Jun 18, 2021 •

edited

Loading