Skip to content

Conversation

@AlexeySachkov
Copy link
Contributor

@AlexeySachkov AlexeySachkov commented Oct 1, 2024

This PR introduces tests for user-defined data types such as structs

@AlexeySachkov AlexeySachkov requested a review from a team as a code owner October 1, 2024 14:22
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

LGTM!

@steffenlarsen steffenlarsen merged commit 17e8f16 into intel:sycl Oct 2, 2024
12 checks passed
@steffenlarsen
Copy link
Contributor

There are some odd build failures in post-commit after this, but I don't see how they are related. Most of the failures seem to come from ur_api.h. @kbenzie - Do you know what is going on?

@kbenzie
Copy link
Contributor

kbenzie commented Oct 2, 2024

There are some odd build failures in post-commit after this, but I don't see how they are related. Most of the failures seem to come from ur_api.h. @kbenzie - Do you know what is going on?

Have you got a link to the failing workflow?

@steffenlarsen
Copy link
Contributor

Have you got a link to the failing workflow?

Yeah, sorry: https://github.com/intel/llvm/actions/runs/11138436547/job/30953406311. Note though that there was a UR bump after which seems to build, so maybe it was just a false alarm.

@kbenzie
Copy link
Contributor

kbenzie commented Oct 2, 2024

That's very odd, I've not seen that before. This is a self build so using clang, right?

@steffenlarsen
Copy link
Contributor

That's very odd, I've not seen that before. This is a self build so using clang, right?

Yeah. It should be using the nightly, AFAIK.

@kbenzie
Copy link
Contributor

kbenzie commented Oct 2, 2024

Note though that there was a UR bump after which seems to build, so maybe it was just a false alarm.

It's like the file was truncated somehow, we'd certainly never remove the #endif of the include guard intentionally. Let me know if it persists and we can look into it further.

@AlexeySachkov AlexeySachkov deleted the private/asachkov/more-raw-kernel-arg-tests branch October 9, 2024 08:01
@AlexeySachkov
Copy link
Contributor Author

Posting error log here for reference, because it will be removed at some point:

[2067/8738] Building CXX object _deps/unified-runtime-build/source/adapters/hip/CMakeFiles/ur_adapter_hip.dir/program.cpp.o
FAILED: _deps/unified-runtime-build/source/adapters/hip/CMakeFiles/ur_adapter_hip.dir/program.cpp.o 
ccache /opt/sycl/bin/clang++ -DSYCL_ENABLE_KERNEL_FUSION -DUR_ENABLE_SANITIZER -DUR_ENABLE_TRACING -DUR_VALIDATION_LAYER_SUPPORTED_VERSION=\"0\" -DUR_VERSION=\"0\" -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D__HIP_PLATFORM_AMD__ -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Dur_adapter_hip_EXPORTS -I/__w/llvm/llvm/build/_deps/unified-runtime-build/source/adapters/hip -I/__w/llvm/llvm/build/_deps/unified-runtime-src/source/adapters/hip -I/__w/llvm/llvm/build/include -I/__w/llvm/llvm/src/llvm/include -I/__w/llvm/llvm/build/_deps/unified-runtime-src/source/adapters/hip/../.. -I/__w/llvm/llvm/build/_deps/unified-runtime-src/include -I/__w/llvm/llvm/build/_deps/unified-runtime-src/source/common -I/__w/llvm/llvm/build/_deps/unified-memory-framework-src/include -I/__w/llvm/llvm/build/_deps/unified-memory-framework-src/src -I/__w/llvm/llvm/build/_deps/unified-memory-framework-src/src/critnib -I/__w/llvm/llvm/build/_deps/unified-memory-framework-src/src/provider -I/__w/llvm/llvm/build/_deps/unified-memory-framework-src/src/memspaces -I/__w/llvm/llvm/build/_deps/unified-memory-framework-src/src/memory_targets -I/__w/llvm/llvm/build/_deps/unified-memory-framework-src/include/umf/pools -isystem /opt/rocm/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Wno-covered-switch-default -Wno-error -O3 -DNDEBUG -fPIC -fPIC -Wall -Wpedantic -Wempty-body -Wunused-parameter -fcolor-diagnostics -fvisibility=hidden -Wno-deprecated-declarations -std=c++17 -MD -MT _deps/unified-runtime-build/source/adapters/hip/CMakeFiles/ur_adapter_hip.dir/program.cpp.o -MF _deps/unified-runtime-build/source/adapters/hip/CMakeFiles/ur_adapter_hip.dir/program.cpp.o.d -o _deps/unified-runtime-build/source/adapters/hip/CMakeFiles/ur_adapter_hip.dir/program.cpp.o -c /__w/llvm/llvm/build/_deps/unified-runtime-src/source/adapters/hip/program.cpp
In file included from /__w/llvm/llvm/build/_deps/unified-runtime-src/source/adapters/hip/program.cpp:11:
In file included from /__w/llvm/llvm/build/_deps/unified-runtime-src/source/adapters/hip/program.hpp:12:
/__w/llvm/llvm/build/include/ur_api.h:13:2: error: unterminated conditional directive
   13 | #ifndef UR_API_H_INCLUDED
      |  ^

Note though that there was a UR bump after which seems to build, so maybe it was just a false alarm.

It's like the file was truncated somehow, we'd certainly never remove the #endif of the include guard intentionally. Let me know if it persists and we can look into it further.

@kbenzie, the file was definitely truncated and we have a bug somewhere in intel/llvm CMake files. Take a closer look at the log: we are building a UR adapter, but the error message comes from build/include/ur_api.h, which is weird. That location is outside of UR, why is it even used?

I mean, we do have -I build/include in the compiler argument, but that seems wrong. I haven't pinpointed the exact place where it happens yet, but we do somehow propagate SYCL includes down to UR build and that's incorrect.

What happens here is that we rewrite that file (there is a custom command which copies _deps/.../include/ur_api.h into build/include/ur_api.h - that causes the file to be truncated. On bad days I can reproduce the issue on every second build (200+ threads).

@kbenzie
Copy link
Contributor

kbenzie commented Nov 25, 2024

@kbenzie, the file was definitely truncated and we have a bug somewhere in intel/llvm CMake files. Take a closer look at the log: we are building a UR adapter, but the error message comes from build/include/ur_api.h, which is weird. That location is outside of UR, why is it even used?

I mean, we do have -I build/include in the compiler argument, but that seems wrong. I haven't pinpointed the exact place where it happens yet, but we do somehow propagate SYCL includes down to UR build and that's incorrect.

What happens here is that we rewrite that file (there is a custom command which copies _deps/.../include/ur_api.h into build/include/ur_api.h - that causes the file to be truncated. On bad days I can reproduce the issue on every second build (200+ threads).

This sounds like a dependency issue in the copying of ur_api.h into build/include to me. That's something the SYCL build system does, I don't know why that happens. Do you have any insight into that?

Seems to me that not copying ur_api.h, and using the include directory pointing to the UR repo, would completely remove the chance of the header being used by another process while it's in the process of being copied.

@AlexeySachkov
Copy link
Contributor Author

This sounds like a dependency issue in the copying of ur_api.h into build/include to me. That's something the SYCL build system does, I don't know why that happens. Do you have any insight into that?

SYCL headers include ur_api.h, which means that we have to make this header available during -fsycl compilation. What we do to achieve that is we copy UR headers which we include from public SYCL headers into a folder with other headers to make it available.

Even though we don't need that for SYCL library build (we can point include paths directly into UR), I still think that we need that copy to make clang++ -fsycl work from build/ directory.

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