[SYCL][NFC] Use -fsyntax-only more extensively in LIT tests #8144

AlexeySachkov · 2023-01-30T09:31:22Z

Adjusted LIT tests to use syntax-only mode for some tests which perform full compilation and linking or only compilation (-c) to speed up those tests.

Also removed -o option from some existing -fsyntax-only mode to shorten RUN lines, because it is anyway ignored by the compiler.

Refactored some tests for predefined macro to make checks at compile-time and not at runtime.

Adjusted LIT tests to use syntax-only mode for some tests which perform full compilation and linking or only compilation (`-c`) to speed up those tests. Also removed `-o` option from some existing `-fsyntax-only` mode to shorten `RUN` lines, because it is anyway ignored by the compiler.

bader

FYI: sometimes, the intention can be to test compiler's IR generation as well.
I don't know if any of the tests you updated requires this.

sycl/test/extensions/macro_esimd_emulator.cpp

AlexeySachkov · 2023-01-30T16:52:23Z

FYI: sometimes, the intention can be to test compiler's IR generation as well. I don't know if any of the tests you updated requires this.

Yes, I'm aware of such tests and haven't touched any of them in this PR.

bader · 2023-01-30T16:53:22Z

Okay, did you look at extensions/macro_cuda.cpp failure?

AlexeySachkov · 2023-01-30T16:57:38Z

Okay, did you look at extensions/macro_cuda.cpp failure?

I think I made a mistake by moving macro check above sycl/sycl.hpp include. I will take a look at macro definition and fix the test.

UPD: should be fixed by 58e43ed

aelovikov-intel

One issue I see with this approach is inability to verify all explicit instantiations are in place. Missing ones manifest themselves at link time only with undefined symbols.

aelovikov-intel · 2023-01-30T17:12:19Z

sycl/test/basic_tests/interop-cuda.cpp

-// RUN: %clangxx %fsycl-host-only -fsyntax-only -Xclang -verify -Xclang -verify-ignore-unexpected=note -DSYCL_EXT_ONEAPI_BACKEND_CUDA_EXPERIMENTAL %s -o %t.out
-// RUN: %clangxx %fsycl-host-only -fsyntax-only -Xclang -verify -Xclang -verify-ignore-unexpected=note -D__SYCL_INTERNAL_API -DSYCL_EXT_ONEAPI_BACKEND_CUDA_EXPERIMENTAL %s -o %t.out
+// RUN: %clangxx %fsycl-host-only -fsyntax-only -Xclang -verify -Xclang -verify-ignore-unexpected=note -DSYCL_EXT_ONEAPI_BACKEND_CUDA_EXPERIMENTAL %s
+// RUN: %clangxx %fsycl-host-only -fsyntax-only -Xclang -verify -Xclang -verify-ignore-unexpected=note -D__SYCL_INTERNAL_API -DSYCL_EXT_ONEAPI_BACKEND_CUDA_EXPERIMENTAL %s


Not related to this particular file, it's just when I realized it... What if a mistake is done inside the pi.def-related mapping and some symbol/offset is missing, would -fsyntax-only catch that?

If I understand correctly, it is likely that SYCL RT (or unit-tests) won't be built at all.

For example, if you add something new into pi.def, but not anywhere else, then the following piece of code will try to reference non-existing function:

llvm/sycl/include/sycl/detail/pi.h

Lines 1982 to 1985 in e833806

struct FunctionPointers {

#define _PI_API(api) decltype(::api) *api;

#include <sycl/detail/pi.def>

} PiFunctionTable;

aelovikov-intel · 2023-01-30T17:17:07Z

sycl/test/basic_tests/accessor/atomic_zero_dimension_accessor.cpp

I'm surprised to see dynamic asserts in this test (lines 56/57). Unrelated to this PR though.

The test was added like this in #4458. We should probably be more careful when reviewing tests and perhaps we should do another round of tests review to refactor them, i.e.:

move to llvm-test-suite anything which requires actual devices

and/or rewrite that to unit-tests

remove runtime checks from -fsyntax-only tests to simplify and cleanup them

aelovikov-intel · 2023-01-30T17:17:39Z

sycl/test/regression/operator-names.cpp

@@ -1,4 +1,4 @@
-// RUN: %clangxx -fsycl -c %s -o %t.out -Wno-deprecated -fno-operator-names
+// RUN: %clangxx -fsycl -fsyntax-only %s -Wno-deprecated -fno-operator-names


it does not appear to respect the -fno-operator-names option

What does it mean? Modified test passes.

I suggest we a use of operator name in the test itself and add --verify option with checks that operator names are used in the test, but no detected in the headers.

BTW, I suppose DPC++ extensions, which are not implicitly included by <sycl/sycl.hpp> should be tested as well. Right?

@Pennycook, FYI.

It seems like the comment on line 7 is not correct. From my local experiments:

$ ./bin/clang++ -fsyntax-only t.cpp -fno-operator-names t.cpp:2:13: error: expected ')' if (a > b and b >c ) { ^ t.cpp:2:6: note: to match this '(' if (a > b and b >c ) { ^ 1 error generated. $ ./bin/clang++ -fsyntax-only t.cpp -foperator-names $ echo $? 0

UPD: -fsycl doesn't alter the behavior, -fno-operator-names still works as expected with it.

Removed the comment in b29ae74

AlexeySachkov requested a review from a team as a code owner January 30, 2023 09:31

AlexeySachkov requested a review from aelovikov-intel January 30, 2023 09:31

AlexeySachkov mentioned this pull request Jan 30, 2023

[SYCL][NFC] Reduce amount of includes in in-tree tests #7438

Closed

AlexeySachkov temporarily deployed to aws January 30, 2023 09:44 — with GitHub Actions Inactive

bader reviewed Jan 30, 2023

View reviewed changes

sycl/test/extensions/macro_esimd_emulator.cpp Outdated Show resolved Hide resolved

sycl/test/extensions/macro_esimd_emulator.cpp Outdated Show resolved Hide resolved

Fix typo

dc8fee6

Fix macro_cuda.cpp test

58e43ed

aelovikov-intel reviewed Jan 30, 2023

View reviewed changes

aelovikov-intel approved these changes Jan 30, 2023

View reviewed changes

AlexeySachkov temporarily deployed to aws January 30, 2023 17:43 — with GitHub Actions Inactive

AlexeySachkov temporarily deployed to aws January 30, 2023 19:05 — with GitHub Actions Inactive

AlexeySachkov temporarily deployed to aws January 30, 2023 21:07 — with GitHub Actions Inactive

AlexeySachkov temporarily deployed to aws January 30, 2023 21:45 — with GitHub Actions Inactive

Remove outdated comment

b29ae74

AlexeySachkov temporarily deployed to aws January 31, 2023 11:22 — with GitHub Actions Inactive

AlexeySachkov temporarily deployed to aws January 31, 2023 11:53 — with GitHub Actions Inactive

AlexeySachkov merged commit 7f24cac into intel:sycl Jan 31, 2023

AlexeySachkov deleted the private/asachkov/use-fsyntax-only branch March 29, 2023 12:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYCL][NFC] Use -fsyntax-only more extensively in LIT tests #8144

[SYCL][NFC] Use -fsyntax-only more extensively in LIT tests #8144

Uh oh!

AlexeySachkov commented Jan 30, 2023

Uh oh!

bader left a comment

Uh oh!

Uh oh!

Uh oh!

AlexeySachkov commented Jan 30, 2023

Uh oh!

bader commented Jan 30, 2023

Uh oh!

AlexeySachkov commented Jan 30, 2023 •

edited

Loading

Uh oh!

aelovikov-intel left a comment

Uh oh!

aelovikov-intel Jan 30, 2023

Uh oh!

AlexeySachkov Jan 31, 2023

Uh oh!

aelovikov-intel Jan 30, 2023

Uh oh!

AlexeySachkov Jan 31, 2023

Uh oh!

aelovikov-intel Jan 30, 2023

Uh oh!

bader Jan 30, 2023

Uh oh!

AlexeySachkov Jan 31, 2023 •

edited

Loading

Uh oh!

AlexeySachkov Jan 31, 2023

Uh oh!

Uh oh!

	struct FunctionPointers {
	#define _PI_API(api) decltype(::api) *api;
	#include <sycl/detail/pi.def>
	} PiFunctionTable;

		@@ -1,4 +1,4 @@
		// RUN: %clangxx -fsycl -c %s -o %t.out -Wno-deprecated -fno-operator-names
		// RUN: %clangxx -fsycl -fsyntax-only %s -Wno-deprecated -fno-operator-names

[SYCL][NFC] Use -fsyntax-only more extensively in LIT tests #8144

[SYCL][NFC] Use -fsyntax-only more extensively in LIT tests #8144

Uh oh!

Conversation

AlexeySachkov commented Jan 30, 2023

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AlexeySachkov commented Jan 30, 2023

Uh oh!

bader commented Jan 30, 2023

Uh oh!

AlexeySachkov commented Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aelovikov-intel left a comment

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel Jan 30, 2023

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel Jan 30, 2023

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

aelovikov-intel Jan 30, 2023

Choose a reason for hiding this comment

Uh oh!

bader Jan 30, 2023

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlexeySachkov Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

AlexeySachkov commented Jan 30, 2023 •

edited

Loading

AlexeySachkov Jan 31, 2023 •

edited

Loading