Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cts] stop device binaries build from silently failing #848

Closed
wants to merge 2 commits into from
Closed

[cts] stop device binaries build from silently failing #848

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0357f88
[cts] build device binaries only when target triplets are set
pbalcer Sep 5, 2023
164825e
[cts] fail the build if device binaries cannot be correctly dumped
pbalcer Sep 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,19 @@

## Table of contents

1. [Contents of the repo](#contents-of-the-repo)
2. [Integration](#integration)
- [Unified Runtime](#unified-runtime)
- [Table of contents](#table-of-contents)
- [Contents of the repo](#contents-of-the-repo)
- [Integration](#integration)
- [Weekly tags](#weekly-tags)
3. [Third-Party tools](#third-party-tools)
4. [Building](#building)
- [Third-Party tools](#third-party-tools)
- [Building](#building)
- [Requirements](#requirements)
- [Windows](#windows)
- [Linux](#linux)
- [CMake standard options](#cmake-standard-options)
- [Additional make targets](#additional-make-targets)
5. [Contributions](#contributions)
- [Contributions](#contributions)
- [Adapter naming convention](#adapter-naming-convention)
- [Source code generation](#source-code-generation)
- [Documentation](#documentation)
Expand Down Expand Up @@ -118,7 +120,7 @@ List of options provided by CMake:
| UR_USE_UBSAN | Enable UndefinedBehavior Sanitizer | ON/OFF | OFF |
| UR_USE_MSAN | Enable MemorySanitizer (clang only) | ON/OFF | OFF |
| UR_ENABLE_TRACING | Enable XPTI-based tracing layer | ON/OFF | OFF |
| UR_CONFORMANCE_TARGET_TRIPLES | SYCL triples to build CTS device binaries for | Comma-separated list | spir64 |
| UR_CONFORMANCE_TARGET_TRIPLES | SYCL triples to build CTS device binaries for | Comma-separated list | |

### Additional make targets

Expand Down
6 changes: 3 additions & 3 deletions test/conformance/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ add_subdirectory(queue)
add_subdirectory(sampler)
add_subdirectory(virtual_memory)


if(DEFINED UR_DPCXX)
add_custom_target(generate_device_binaries)

Expand All @@ -66,12 +67,11 @@ if(DEFINED UR_DPCXX)

if(NOT "${UR_CONFORMANCE_TARGET_TRIPLES}" STREQUAL "")
string(REPLACE "," ";" TARGET_TRIPLES ${UR_CONFORMANCE_TARGET_TRIPLES})
add_subdirectory(device_code)
else()
message(WARNING "UR_CONFORMANCE_TARGET_TRIPLES wasn't set, defaulting to only generate spir64 device binaries")
list(APPEND TARGET_TRIPLES "spir64")
message(WARNING "UR_CONFORMANCE_TARGET_TRIPLES wasn't set, device binaries will not be generated.")
endif()

add_subdirectory(device_code)
add_subdirectory(kernel)
Copy link
Contributor

Choose a reason for hiding this comment

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

UR_DPCXX is only required for device_code stuff iirc, we could move all these out of the if altogether as long as we're content to build them without device code

add_subdirectory(program)
add_subdirectory(enqueue)
Expand Down
2 changes: 1 addition & 1 deletion test/conformance/device_code/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ macro(add_device_binary SOURCE_FILE)
COMMAND ${UR_DPCXX} -fsycl -fsycl-targets=${TRIPLE} -fsycl-device-code-split=off
${SOURCE_FILE} -o ${EXE_PATH}
COMMAND ${CMAKE_COMMAND} -E env SYCL_DUMP_IMAGES=true
${EXE_PATH} || (exit 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

We're going to want a mechanism that filters which programs to build for which backends before doing this, as it stands I can build and run the program tests for e.g. l0 even though a number of the executables segfault presumably due to stuff like spec constants and images being unsupported. Possibly worth noting that despite the segfaults the IR still dumps. Maybe what we want instead for the short term is something to find libsycl at configure time so that particular common issue with these doesn't go unnoticed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point... The build also fails right now because generating kernel_entry_points.h is part of the device_code cmake.

Approaching this from a different side, there are two 'build criteria' I see for kernel tests. First, as you've noted, ld needs to be able to find libsycl, and the second is having a device capable of running the kernels for the triplet.
If we can't rely on the binary failing, I think we need to detect those criteria prior to running it and emit a relevant message.

${EXE_PATH}
WORKING_DIRECTORY "${DEVICE_BINARY_DIR}"
DEPENDS ${SOURCE_FILE}
)
Expand Down
Loading