[SYCL][RTC] Clarify and test handling of include paths #17307

jopperm · 2025-03-05T00:24:01Z

Multiple changes related to the handling of #include's in RTC:

Fix a bug in how the in-memory pipeline rendered joined arguments
Add "current working directory" and "directories specified via -I in build_options" to the locations that extension will look for header files, and clarifiy search order, in the specification. This change is meant to establish clang's usual include behavior.
Add an extensive E2E test for include handling
Check if filename is not present in include_files::add to match specification
Add missing constructors and add method to include_files and build_properties to match specification

Signed-off-by: Julian Oppermann <[email protected]>

sycl/doc/extensions/experimental/sycl_ext_oneapi_kernel_compiler.asciidoc

Pennycook · 2025-03-05T10:21:16Z

sycl/doc/extensions/experimental/sycl_ext_oneapi_kernel_compiler.asciidoc

+is useful, for example, to compile kernels using external libraries. The
+following spellings are supported (note that the path is a separate element in
+the `build_options` list for the second and fourth form):
+
+[source,c++]
+----
+build_options{{
+  {"-Ipath"},
+  {"-I"}, {"path"},
+  {"--include-directory=path"},
+  {"--include-directory"}, {"path"}
+}}
+----


Nit: I'd prefer it if we actually documented each of these flags separately, but I'm not sure we have precedent for how to do that. I don't want this comment to block this PR, but I'm leaving a note for @gmlueck to review upon his return.

FWIW, these are standard compiler flags for Clang and GCC, not specific to RTC, so users are potentially already familiar with them.

That's a good point. I agree that most people reading the specification will be able to figure this out, and that's another reason not to let my comment completely block this PR.

But, the specification for build_options currently says:

The source_language::sycl language does not define any standard build options, but an implementation may support implementation-defined options.

Because the current description of these include options is so vague, it's not clear to me whether there are intended to be supported by all implementations of this extension (in which case, the description of build_options is wrong), or whether they are intended to be specific to DPC++.

The only situation I'd feel comfortable leaving things with a vague description would be if the specification said something like "In DPC++, any additional build_options behave as if passed to DPC++", in which case we'd inherit the definitions of -I etc from there. But I don't know if that's the current behavior.

I agree, the documentation of the build_options still needs some attention in this document.

"In DPC++, any additional build_options behave as if passed to DPC++"

This is true for many options, but there are definitely some we cannot support right now (e.g. everything related to AoT compilation).

I plan to add a list of supported options before the release, in form of a DPC++-specific, non-normative section* towards the end of the document. This would of course include the -I option, so we could move the examples for using it to that section, and/or reference the DPC++ docs. Makes sense to me that the specification part would then only state that implementation-specific options can be added to build_options to include additional paths, and point the reader to the non-normative section.

*) Greg suggested (or at least approved of) having such a section.

Okay, thanks for explaining that. I think if the intent is that -I is not guaranteed to be supported by all compilers, we should create the DPC++-specific non-normative section now, and just put the -I flags in it.

It's much easier to take something that's DPC++-specific and make it available to all implementations than to do the opposite. And you can still add the documentation for the other flags later.

[...] we should create the DPC++-specific non-normative section now, and just put the -I flags in it.

Makes sense, done! @Pennycook can you have another look, please?

sycl/doc/extensions/experimental/sycl_ext_oneapi_kernel_compiler.asciidoc

sommerlukas · 2025-03-05T14:11:21Z

sycl/doc/extensions/experimental/sycl_ext_oneapi_kernel_compiler.asciidoc

+is useful, for example, to compile kernels using external libraries. The
+following spellings are supported (note that the path is a separate element in
+the `build_options` list for the second and fourth form):
+
+[source,c++]
+----
+build_options{{
+  {"-Ipath"},
+  {"-I"}, {"path"},
+  {"--include-directory=path"},
+  {"--include-directory"}, {"path"}
+}}
+----


FWIW, these are standard compiler flags for Clang and GCC, not specific to RTC, so users are potentially already familiar with them.

sommerlukas · 2025-03-05T14:13:48Z

sycl/include/sycl/kernel_bundle.hpp

+    if (std::find_if(record.begin(), record.end(), [&name](auto &p) {
+          return p.first == name;
+        }) != record.end()) {
+      throw sycl::exception(make_error_code(errc::invalid),
+                            "Include file '" + name +
+                                "' is already registered");
+    }


Does the specification promise this exception to be thrown?

It seems like potential overhead to go through the existing list over and over again.

Yes:

llvm/sycl/doc/extensions/experimental/sycl_ext_oneapi_kernel_compiler.asciidoc

Lines 381 to 382 in b02a2c1

* An `exception` with the `errc::invalid` error code if there is already an

entry with `name` in this property.

Not sure if we are expecting apps to register many virtual header files. For registered_kernel_names, the semantics are more relaxed:

llvm/sycl/doc/extensions/experimental/sycl_ext_oneapi_kernel_compiler.asciidoc

Lines 522 to 523 in b02a2c1

[_Note:_ It is not an error to have duplicate names in a `registered_names`

property, but the duplicates have no effect.

Maybe we should adopt that. WDYT?

Could we potentially use a different data structure, e.g., map, to make the lookup more efficient and avoid the quadratic complexity?

Yes, the order of the virtual include files should be insignificant. In 0ad484d I switched the container to an unordered_map. Unfortunately this causes the rather large change to the sycl_classes_abi_neutral_test.cpp test.

…er.asciidoc Co-authored-by: John Pennycook <[email protected]>

Signed-off-by: Julian Oppermann <[email protected]>

jopperm · 2025-03-10T09:27:48Z

Changes themselves LGTM, but we need to check whether include_files has been in a release ABI and if we can make this potentially ABI-breaking change now.

AFAICT this is not an ABI break. The entrypoint into libsycl is:

llvm/sycl/include/sycl/kernel_bundle.hpp

Lines 1027 to 1032 in a412c12

    
           __SYCL_EXPORT kernel_bundle<bundle_state::ext_oneapi_source> 
        
           make_kernel_bundle_from_source( 
        
               const context &SyclContext, source_language Language, 
        
               sycl::detail::string_view Source, 
        
               std::vector<std::pair<sycl::detail::string_view, sycl::detail::string_view>> 
        
                   IncludePairsVec);

which is unaffected by the changes to include_files.

sycl/test/abi/sycl_classes_abi_neutral_test.cpp ensures that SYCL classes do not contain std::basic_string and std::list members. include_files was already listed as an exclusion in the test, however, a map's internal layout is more complicated than a vector, requiring more CHECK lines to match all occurences of std::basic_string.

sommerlukas

sycl/test/abi/sycl_classes_abi_neutral_test.cpp ensures that SYCL classes do not contain std::basic_string and std::list members. include_files was already listed as an exclusion in the test, however, a map's internal layout is more complicated than a vector, requiring more CHECK lines to match all occurences of std::basic_string.

Thanks for explanation, that makes sense.

Implementation LGTM.

Signed-off-by: Julian Oppermann <[email protected]>

Pennycook

Latest extension specification LGTM.

Signed-off-by: Julian Oppermann <[email protected]>

jopperm · 2025-03-11T21:53:42Z

@intel/llvm-gatekeepers Can you merge this, please?

I'd argue we don't need to wait for the AMD runner. The AMD workflow was successful for 5fbd5b0, and I just bumped the counter in a SYCL lit test for the pending commit.

Partially revert #17307: change the container in the `include_files` property back to vector of pairs, as the more complex internal structure of a map made it very fragile to test in `sycl_classes_abi_neutral_test.cpp`. --------- Signed-off-by: Julian Oppermann <[email protected]>

jopperm added 4 commits March 5, 2025 00:20

Fix argument rendering

52f61b5

Signed-off-by: Julian Oppermann <[email protected]>

Add E2E test

a634597

Signed-off-by: Julian Oppermann <[email protected]>

Merge remote-tracking branch 'upstream/sycl' into rtc-includes

07e46f2

Bump sycl.hpp counter

42172e9

Signed-off-by: Julian Oppermann <[email protected]>

jopperm self-assigned this Mar 5, 2025

jopperm temporarily deployed to WindowsCILock March 5, 2025 00:24 — with GitHub Actions Inactive

jopperm had a problem deploying to WindowsCILock March 5, 2025 00:58 — with GitHub Actions Failure

Fix properties

3435455

Signed-off-by: Julian Oppermann <[email protected]>

jopperm temporarily deployed to WindowsCILock March 5, 2025 01:57 — with GitHub Actions Inactive

jopperm had a problem deploying to WindowsCILock March 5, 2025 02:26 — with GitHub Actions Failure

Check each name can only be used once

95cce0d

Signed-off-by: Julian Oppermann <[email protected]>

jopperm had a problem deploying to WindowsCILock March 5, 2025 03:51 — with GitHub Actions Error

Mention CWD and -I in spec

83c948a

Signed-off-by: Julian Oppermann <[email protected]>

jopperm temporarily deployed to WindowsCILock March 5, 2025 04:12 — with GitHub Actions Inactive

Format

69bb55f

Signed-off-by: Julian Oppermann <[email protected]>

jopperm had a problem deploying to WindowsCILock March 5, 2025 04:42 — with GitHub Actions Error

Windows-compatible absolute paths

5e8c441

Signed-off-by: Julian Oppermann <[email protected]>

jopperm temporarily deployed to WindowsCILock March 5, 2025 04:54 — with GitHub Actions Inactive

jopperm temporarily deployed to WindowsCILock March 5, 2025 05:24 — with GitHub Actions Inactive

jopperm marked this pull request as ready for review March 5, 2025 07:56

jopperm requested review from a team as code owners March 5, 2025 07:56

jopperm requested review from uditagarwal97 and cperkinsintel March 5, 2025 07:56

Pennycook requested changes Mar 5, 2025

View reviewed changes

sommerlukas reviewed Mar 5, 2025

View reviewed changes

jopperm and others added 2 commits March 6, 2025 13:09

Update sycl/doc/extensions/experimental/sycl_ext_oneapi_kernel_compil…

6fb1cf8

…er.asciidoc Co-authored-by: John Pennycook <[email protected]>

Update sycl/doc/extensions/experimental/sycl_ext_oneapi_kernel_compil…

be13943

…er.asciidoc Co-authored-by: John Pennycook <[email protected]>

jopperm had a problem deploying to WindowsCILock March 6, 2025 00:10 — with GitHub Actions Error

jopperm force-pushed the rtc-includes branch from e919c57 to 7dd1e4b Compare March 10, 2025 02:49

jopperm temporarily deployed to WindowsCILock March 10, 2025 02:50 — with GitHub Actions Inactive

jopperm temporarily deployed to WindowsCILock March 10, 2025 03:23 — with GitHub Actions Inactive

Make ancient GCC happy

165c3d1

Signed-off-by: Julian Oppermann <[email protected]>

jopperm temporarily deployed to WindowsCILock March 10, 2025 07:15 — with GitHub Actions Inactive

jopperm temporarily deployed to WindowsCILock March 10, 2025 07:47 — with GitHub Actions Inactive

sommerlukas approved these changes Mar 10, 2025

View reviewed changes

cperkinsintel approved these changes Mar 10, 2025

View reviewed changes

jopperm added 3 commits March 10, 2025 20:30

Merge remote-tracking branch 'upstream/sycl' into rtc-includes

c6183d7

Move notes on -I flags to new non-normative section

4f6406d

Signed-off-by: Julian Oppermann <[email protected]>

Asciidoc

f008e0a

Signed-off-by: Julian Oppermann <[email protected]>

jopperm temporarily deployed to WindowsCILock March 10, 2025 21:30 — with GitHub Actions Inactive

jopperm temporarily deployed to WindowsCILock March 10, 2025 21:45 — with GitHub Actions Inactive

Pennycook approved these changes Mar 11, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/sycl' into rtc-includes

5fbd5b0

jopperm temporarily deployed to WindowsCILock March 11, 2025 09:58 — with GitHub Actions Inactive

jopperm temporarily deployed to WindowsCILock March 11, 2025 10:13 — with GitHub Actions Inactive

Bump sycl.hpp counter

a111959

Signed-off-by: Julian Oppermann <[email protected]>

jopperm temporarily deployed to WindowsCILock March 11, 2025 19:19 — with GitHub Actions Inactive

jopperm temporarily deployed to WindowsCILock March 11, 2025 19:49 — with GitHub Actions Inactive

jopperm temporarily deployed to WindowsCILock March 11, 2025 19:50 — with GitHub Actions Inactive

uditagarwal97 merged commit 81e9be7 into intel:sycl Mar 11, 2025
24 checks passed

jopperm mentioned this pull request Mar 14, 2025

[SYCL][RTC][NFC] Revert changes to include_files property #17460

Merged

	* An `exception` with the `errc::invalid` error code if there is already an
	entry with `name` in this property.

	[_Note:_ It is not an error to have duplicate names in a `registered_names`
	property, but the duplicates have no effect.

[SYCL][RTC] Clarify and test handling of include paths #17307

[SYCL][RTC] Clarify and test handling of include paths #17307

Uh oh!

Conversation

jopperm commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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!

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!

jopperm Mar 7, 2025 • 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!

jopperm commented Mar 10, 2025

Uh oh!

sommerlukas left a comment

Choose a reason for hiding this comment

Uh oh!

Pennycook left a comment

Choose a reason for hiding this comment

Uh oh!

jopperm commented Mar 11, 2025

Uh oh!

Uh oh!

Uh oh!

jopperm commented Mar 5, 2025 •

edited

Loading

jopperm Mar 7, 2025 •

edited

Loading