Skip to content

Conversation

@steffenlarsen
Copy link
Contributor

This commit sorts the included files in the include_deps tests of KHR include headers. This should help ensure consistency in testing, while preserving the boundaries of the includes.

This commit sorts the included files in the include_deps tests of KHR
include headers. This should help ensure consistency in testing, while
preserving the boundaries of the includes.

Signed-off-by: Larsen, Steffen <steffen.larsen@intel.com>
Copy link
Contributor

@aelovikov-intel aelovikov-intel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had it unsorted on purpose, because a header moved later in the test indicated a positive change even if the total number remained the same.

What problem are you trying to solve?

@steffenlarsen
Copy link
Contributor Author

I had it unsorted on purpose, because a header moved later in the test indicated a positive change even if the total number remained the same.

What problem are you trying to solve?

The test is failing locally due to the cstdlib include being in another place in the list. Seemed reasonable based on that that ordering did not matter, but if it is intentional maybe we need another solution.

@AlexeySachkov
Copy link
Contributor

Surprisingly, I can reproduce the failure locally as well:

# RUN: at line 3
bash intel/llvm/sycl/test/include_deps/deps_known.sh sycl/khr/includes/usm.hpp | FileCheck intel/llvm/sycl/test/include_d
eps/sycl_khr_includes_usm.hpp.cpp
# executed command: bash intel/llvm/sycl/test/include_deps/deps_known.sh sycl/khr/includes/usm.hpp
# executed command: FileCheck intel/llvm/sycl/test/include_deps/sycl_khr_includes_usm.hpp.cpp
# .---command stderr------------
# | intel/llvm/sycl/test/include_deps/sycl_khr_includes_usm.hpp.cpp:38:16: error: CHECK-NEXT: is not on the line after the previous match
# | // CHECK-NEXT: stl_wrappers/cstdlib
# |                ^
# | <stdin>:38:1: note: 'next' match was here
# | stl_wrappers/cstdlib
# | ^
# | <stdin>:33:24: note: previous match ended here
# | detail/vector_arith.hpp
# |                        ^
# | <stdin>:34:1: note: non-matching line after previous match is here
# | detail/common.hpp
# | ^
# |·
# | Input file: <stdin>
# | Check file: intel/llvm/sycl/test/include_deps/sycl_khr_includes_usm.hpp.cpp
# |·
# | -dump-input=help explains the following input dump.
# |·
# | Input was:
# | <<<<<<
# |          .
# |          .
# |          .
# |         33: detail/vector_arith.hpp·
# |         34: detail/common.hpp·
# |         35: stl_wrappers/cassert·
# |         36: stl_wrappers/assert.h·
# |         37: detail/fwd/accessor.hpp·
# |         38: stl_wrappers/cstdlib·
# | next:38     !~~~~~~~~~~~~~~~~~~~  error: match on wrong line
# |         39: marray.hpp·
# |         40: multi_ptr.hpp·
# |         41: detail/address_space_cast.hpp·
# |         42: detail/builtins/common_functions.inc·
# |         43: detail/builtins/helper_macros.hpp·
# |          .
# |          .
# |          .
# | >>>>>>
# `-----------------------------
# error: command failed with exit status: 1

--

The fact that we do not include <cstdlib> directly almost anywhere makes me think that the failure is caused by a different local version of the C++ headers:

$ grep -rn "cstdlib" ../sycl/include/
../sycl/include/sycl/detail/os_util.hpp:16:#include <cstdlib> // for size_t
../sycl/include/sycl/usm/usm_allocator.hpp:19:#include <cstdlib>     // for size_t, aligned_alloc, free
../sycl/include/sycl/stl_wrappers/cstdlib:1://==- cstdlib --------------------------------------------------------------==//
../sycl/include/sycl/stl_wrappers/cstdlib:11:// Include real STL <cstdlib> header - the next one from the include search
../sycl/include/sycl/stl_wrappers/cstdlib:15:#include_next <cstdlib>
../sycl/include/sycl/stl_wrappers/cstdlib:18:// Our header is located in "stl_wrappers/cstdlib" so it won't be picked by the
../sycl/include/sycl/stl_wrappers/cstdlib:20:// where the following would result in the <cstdlib> we want. This is obviously
../sycl/include/sycl/stl_wrappers/cstdlib:22:#include <../include/cstdlib>

Note that pre-commit, nightly and post-commit all pass.

Before tests for khr includes were introduced, we only checked like 3 header files, but now we check way more so we just uncovered an existing quirkiness.

I wonder if we should just excludestl_wrappers/ subdirectory from those tests

@aelovikov-intel
Copy link
Contributor

I wonder if we should just exclude stl_wrappers/ subdirectory from those tests

+1 if that helps. In theory, our wrappers can trigger an "earlier" inclusion of some sycl/detail/*.hpp, but I think those would be already included by almost any other sycl header.

Alternatively, we can adjust the test itself to include <cstdlib> before the first SYCL header.

@steffenlarsen
Copy link
Contributor Author

Let's go with the proposed solutions instead. 👍

@AlexeySachkov
Copy link
Contributor

I wonder if we should just exclude stl_wrappers/ subdirectory from those tests

+1 if that helps. In theory, our wrappers can trigger an "earlier" inclusion of some sycl/detail/*.hpp, but I think those would be already included by almost any other sycl header.

PR to drop stl_wrappers from check lines: #20480

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