Skip to content

Conversation

@sudonatalie
Copy link
Member

@sudonatalie sudonatalie commented Nov 21, 2023

Add spirv-dis (disassembler) and spirv-val (validator) from SPIRV-Tools as external dependencies for testing the SPIR-V backend. These tools are test dependencies only.

SPIR-V backend tests now have a dependency on the spirv-dis and spirv-val targets when the LLVM_INCLUDE_SPIRV_TOOLS_TESTS cmake variable is set, which allows additional test files with the REQUIRES: spirv-tools constraint to run, along with additional RUN: %if spirv-tools ... lines in existing tests. All other SPIR-V backend tests will run normally when LLVM_INCLUDE_SPIRV_TOOLS_TESTS is not set.

Several tests are included to show these tools' use, however more tests will be migrated and added later.

  • OpVariable_order.ll shows how spirv-val can catch bugs in the backend.
  • basic_int_types_spirvdis.ll shows how tests can be much shorter and more readable by FileChecking the spirv-dis output.
  • basic_int_types.ll shows how an additional RUN line can add validation to existing tests.

RFC: https://discourse.llvm.org/t/rfc-add-a-test-dependency-on-spirv-tools/75135

@michalpaszkowski michalpaszkowski self-requested a review November 22, 2023 11:59
@sudonatalie sudonatalie marked this pull request as ready for review November 29, 2023 20:50
@sudonatalie sudonatalie requested a review from Keenuts November 29, 2023 20:50
@llvmbot
Copy link
Member

llvmbot commented Nov 29, 2023

@llvm/pr-subscribers-backend-spir-v

Author: Natalie Chouinard (sudonatalie)

Changes

Add spirv-dis (disassembler) and spirv-val (validator) from SPIRV-Tools as external dependencies for testing the SPIR-V backend. These tools are test dependencies only.

SPIR-V tests now have a dependency on the spirv-dis and spirv-val targets, and are only built when the LLVM_INCLUDE_SPIRV_TOOLS_TESTS cmake variable is set.

Two tests are included to show these tools' use, however more tests will be migrated and added later.

  • OpVariable_order.ll shows how spirv-val can catch bugs in the backend.
  • basic_int_types.ll shows how tests can be much shorter and more readable by FileChecking the spirv-dis output.

Full diff: https://github.com/llvm/llvm-project/pull/73044.diff

6 Files Affected:

  • (modified) llvm/test/CMakeLists.txt (+5)
  • (added) llvm/test/CodeGen/SPIRV/OpVariable_order.ll (+17)
  • (modified) llvm/test/CodeGen/SPIRV/basic_int_types.ll (+16-44)
  • (modified) llvm/test/CodeGen/SPIRV/lit.local.cfg (+5)
  • (modified) llvm/test/lit.site.cfg.py.in (+1)
  • (added) llvm/tools/spirv-tools/CMakeLists.txt (+66)
diff --git a/llvm/test/CMakeLists.txt b/llvm/test/CMakeLists.txt index 8aa652240081ce5..2ae07ec2df752ba 100644 --- a/llvm/test/CMakeLists.txt +++ b/llvm/test/CMakeLists.txt @@ -222,6 +222,11 @@ if(LLVM_ENABLE_HTTPLIB) list(APPEND LLVM_TEST_DEPENDS llvm-debuginfod) endif() +if (LLVM_INCLUDE_SPIRV_TOOLS_TESTS) + list(APPEND LLVM_TEST_DEPENDS spirv-dis) + list(APPEND LLVM_TEST_DEPENDS spirv-val) +endif() + add_custom_target(llvm-test-depends DEPENDS ${LLVM_TEST_DEPENDS}) set_target_properties(llvm-test-depends PROPERTIES FOLDER "Tests") diff --git a/llvm/test/CodeGen/SPIRV/OpVariable_order.ll b/llvm/test/CodeGen/SPIRV/OpVariable_order.ll new file mode 100644 index 000000000000000..a4ca3aa709f0fab --- /dev/null +++ b/llvm/test/CodeGen/SPIRV/OpVariable_order.ll @@ -0,0 +1,17 @@ +; REQUIRES: spirv-tools +; RUN: llc -O0 -mtriple=spirv-unknown-linux %s -o - -filetype=obj | not spirv-val 2>&1 | FileCheck %s + +; TODO(#66261): The SPIR-V backend should reorder OpVariable instructions so this doesn't fail, +; but in the meantime it's a good example of the spirv-val tool working as intended. + +; CHECK: All OpVariable instructions in a function must be the first instructions in the first block. + +define void @main() #1 { +entry: + %0 = alloca <2 x i32>, align 4 + %1 = getelementptr <2 x i32>, ptr %0, i32 0, i32 0 + %2 = alloca float, align 4 + ret void +} + +attributes #1 = { "hlsl.numthreads"="4,8,16" "hlsl.shader"="compute" } diff --git a/llvm/test/CodeGen/SPIRV/basic_int_types.ll b/llvm/test/CodeGen/SPIRV/basic_int_types.ll index b479cee0bed71cc..3778d897929188b 100644 --- a/llvm/test/CodeGen/SPIRV/basic_int_types.ll +++ b/llvm/test/CodeGen/SPIRV/basic_int_types.ll @@ -1,72 +1,44 @@ -; RUN: llc -O0 -mtriple=spirv-unknown-unknown %s -o - | FileCheck %s -; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - | FileCheck %s -; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - | FileCheck %s +; REQUIRES: spirv-tools +; RUN: llc -O0 -mtriple=spirv-unknown-unknown %s -o - --filetype=obj | spirv-dis | FileCheck %s +; RUN: llc -O0 -mtriple=spirv32-unknown-unknown %s -o - --filetype=obj | spirv-dis | FileCheck %s +; RUN: llc -O0 -mtriple=spirv64-unknown-unknown %s -o - --filetype=obj | spirv-dis | FileCheck %s define void @main() { entry: -; CHECK-DAG: %[[#short:]] = OpTypeInt 16 0 -; CHECK-DAG: %[[#int:]] = OpTypeInt 32 0 -; CHECK-DAG: %[[#long:]] = OpTypeInt 64 0 - -; CHECK-DAG: %[[#v2short:]] = OpTypeVector %[[#short]] 2 -; CHECK-DAG: %[[#v3short:]] = OpTypeVector %[[#short]] 3 -; CHECK-DAG: %[[#v4short:]] = OpTypeVector %[[#short]] 4 - -; CHECK-DAG: %[[#v2int:]] = OpTypeVector %[[#int]] 2 -; CHECK-DAG: %[[#v3int:]] = OpTypeVector %[[#int]] 3 -; CHECK-DAG: %[[#v4int:]] = OpTypeVector %[[#int]] 4 - -; CHECK-DAG: %[[#v2long:]] = OpTypeVector %[[#long]] 2 -; CHECK-DAG: %[[#v3long:]] = OpTypeVector %[[#long]] 3 -; CHECK-DAG: %[[#v4long:]] = OpTypeVector %[[#long]] 4 - -; CHECK-DAG: %[[#ptr_Function_short:]] = OpTypePointer Function %[[#short]] -; CHECK-DAG: %[[#ptr_Function_int:]] = OpTypePointer Function %[[#int]] -; CHECK-DAG: %[[#ptr_Function_long:]] = OpTypePointer Function %[[#long]] -; CHECK-DAG: %[[#ptr_Function_v2short:]] = OpTypePointer Function %[[#v2short]] -; CHECK-DAG: %[[#ptr_Function_v3short:]] = OpTypePointer Function %[[#v3short]] -; CHECK-DAG: %[[#ptr_Function_v4short:]] = OpTypePointer Function %[[#v4short]] -; CHECK-DAG: %[[#ptr_Function_v2int:]] = OpTypePointer Function %[[#v2int]] -; CHECK-DAG: %[[#ptr_Function_v3int:]] = OpTypePointer Function %[[#v3int]] -; CHECK-DAG: %[[#ptr_Function_v4int:]] = OpTypePointer Function %[[#v4int]] -; CHECK-DAG: %[[#ptr_Function_v2long:]] = OpTypePointer Function %[[#v2long]] -; CHECK-DAG: %[[#ptr_Function_v3long:]] = OpTypePointer Function %[[#v3long]] -; CHECK-DAG: %[[#ptr_Function_v4long:]] = OpTypePointer Function %[[#v4long]] - -; CHECK: %[[#]] = OpVariable %[[#ptr_Function_short]] Function +; CHECK: %int16_t_Val = OpVariable %_ptr_Function_ushort Function %int16_t_Val = alloca i16, align 2 -; CHECK: %[[#]] = OpVariable %[[#ptr_Function_int]] Function +; CHECK: %int_Val = OpVariable %_ptr_Function_uint Function %int_Val = alloca i32, align 4 -; CHECK: %[[#]] = OpVariable %[[#ptr_Function_long]] Function +; CHECK: %int64_t_Val = OpVariable %_ptr_Function_ulong Function %int64_t_Val = alloca i64, align 8 -; CHECK: %[[#]] = OpVariable %[[#ptr_Function_v2short]] Function +; CHECK: %int16_t2_Val = OpVariable %_ptr_Function_v2ushort Function %int16_t2_Val = alloca <2 x i16>, align 4 -; CHECK: %[[#]] = OpVariable %[[#ptr_Function_v3short]] Function +; CHECK: %int16_t3_Val = OpVariable %_ptr_Function_v3ushort Function %int16_t3_Val = alloca <3 x i16>, align 8 -; CHECK: %[[#]] = OpVariable %[[#ptr_Function_v4short]] Function +; CHECK: %int16_t4_Val = OpVariable %_ptr_Function_v4ushort Function %int16_t4_Val = alloca <4 x i16>, align 8 -; CHECK: %[[#]] = OpVariable %[[#ptr_Function_v2int]] Function +; CHECK: %int2_Val = OpVariable %_ptr_Function_v2uint Function %int2_Val = alloca <2 x i32>, align 8 -; CHECK: %[[#]] = OpVariable %[[#ptr_Function_v3int]] Function +; CHECK: %int3_Val = OpVariable %_ptr_Function_v3uint Function %int3_Val = alloca <3 x i32>, align 16 -; CHECK: %[[#]] = OpVariable %[[#ptr_Function_v4int]] Function +; CHECK: %int4_Val = OpVariable %_ptr_Function_v4uint Function %int4_Val = alloca <4 x i32>, align 16 -; CHECK: %[[#]] = OpVariable %[[#ptr_Function_v2long]] Function +; CHECK: %int64_t2_Val = OpVariable %_ptr_Function_v2ulong Function %int64_t2_Val = alloca <2 x i64>, align 16 -; CHECK: %[[#]] = OpVariable %[[#ptr_Function_v3long]] Function +; CHECK: %int64_t3_Val = OpVariable %_ptr_Function_v3ulong Function %int64_t3_Val = alloca <3 x i64>, align 32 -; CHECK: %[[#]] = OpVariable %[[#ptr_Function_v4long]] Function +; CHECK: %int64_t4_Val = OpVariable %_ptr_Function_v4ulong Function %int64_t4_Val = alloca <4 x i64>, align 32 ret void diff --git a/llvm/test/CodeGen/SPIRV/lit.local.cfg b/llvm/test/CodeGen/SPIRV/lit.local.cfg index 78dd74cd6dc6349..00f50fb6f1cff7c 100644 --- a/llvm/test/CodeGen/SPIRV/lit.local.cfg +++ b/llvm/test/CodeGen/SPIRV/lit.local.cfg @@ -1,2 +1,7 @@ if not "SPIRV" in config.root.targets: config.unsupported = True + +if config.spirv_tools_tests: + config.available_features.add("spirv-tools") + config.substitutions.append(("spirv-dis", os.path.join(config.llvm_tools_dir, "spirv-dis"))) + config.substitutions.append(("spirv-val", os.path.join(config.llvm_tools_dir, "spirv-val"))) diff --git a/llvm/test/lit.site.cfg.py.in b/llvm/test/lit.site.cfg.py.in index 6f3ab6561c9ec9c..1138b2ccf7bce7e 100644 --- a/llvm/test/lit.site.cfg.py.in +++ b/llvm/test/lit.site.cfg.py.in @@ -60,6 +60,7 @@ config.expensive_checks = @LLVM_ENABLE_EXPENSIVE_CHECKS@ config.reverse_iteration = @LLVM_ENABLE_REVERSE_ITERATION@ config.dxil_tests = @LLVM_INCLUDE_DXIL_TESTS@ config.have_llvm_driver = @LLVM_TOOL_LLVM_DRIVER_BUILD@ +config.spirv_tools_tests = @LLVM_INCLUDE_SPIRV_TOOLS_TESTS@ import lit.llvm lit.llvm.initialize(lit_config, config) diff --git a/llvm/tools/spirv-tools/CMakeLists.txt b/llvm/tools/spirv-tools/CMakeLists.txt new file mode 100644 index 000000000000000..f139e2a46e57c3e --- /dev/null +++ b/llvm/tools/spirv-tools/CMakeLists.txt @@ -0,0 +1,66 @@ +option(LLVM_INCLUDE_SPIRV_TOOLS_TESTS "Include tests that use SPIRV-Tools" Off) +mark_as_advanced(LLVM_INCLUDE_SPIRV_TOOLS_TESTS) + +if (NOT LLVM_INCLUDE_SPIRV_TOOLS_TESTS) + return() +endif () + +if (NOT "SPIRV" IN_LIST LLVM_TARGETS_TO_BUILD) + message(FATAL_ERROR "Building SPIRV-Tools tests is unsupported without the SPIR-V target") +endif () + +# SPIRV_DIS and SPIRV_VAL variables can be used to provide paths to existing +# spirv-dis and spirv-val binaries, respectively. Otherwise, build them from +# SPIRV-Tools source. +if (NOT SPIRV_DIS OR NOT SPIRV_VAL) + include(ExternalProject) + + set(BINARY_DIR ${CMAKE_CURRENT_BINARY_DIR}/SPIRVTools-bin) + + ExternalProject_Add(SPIRVTools + GIT_REPOSITORY https://github.com/KhronosGroup/SPIRV-Tools.git + GIT_TAG main + BINARY_DIR ${BINARY_DIR} + BUILD_COMMAND ${CMAKE_COMMAND} --build ${BINARY_DIR} --target spirv-dis spirv-val + BUILD_BYPRODUCTS ${BINARY_DIR}/tools/spirv-dis ${BINARY_DIR}/tools/spirv-val + # Don't auto-update on every build. + UPDATE_DISCONNECTED 1 + # Allow manual updating with an explicit SPIRVTools-update target. + STEP_TARGETS update + INSTALL_COMMAND "" + ) + + ExternalProject_Add_Step(SPIRVTools get-deps + COMMENT "Getting SPIRV-Tools dependencies" + COMMAND ${Python3_EXECUTABLE} utils/git-sync-deps + DEPENDEES update + WORKING_DIRECTORY <SOURCE_DIR> + ) +endif () + +if (CMAKE_HOST_UNIX) + set(LLVM_LINK_OR_COPY create_symlink) +else () + set(LLVM_LINK_OR_COPY copy) +endif () + +# Link the provided or just built spirv-dis and spirv-val binaries. +if (SPIRV_DIS) + add_custom_target(spirv-dis + COMMAND ${CMAKE_COMMAND} -E ${LLVM_LINK_OR_COPY} "${SPIRV_DIS}" "${LLVM_RUNTIME_OUTPUT_INTDIR}/spirv-dis") +else () + add_custom_target(spirv-dis + COMMAND ${CMAKE_COMMAND} -E ${LLVM_LINK_OR_COPY} "${BINARY_DIR}/tools/spirv-dis" "${LLVM_RUNTIME_OUTPUT_INTDIR}/spirv-dis" + DEPENDS SPIRVTools + ) +endif () + +if (SPIRV_VAL) + add_custom_target(spirv-val + COMMAND ${CMAKE_COMMAND} -E ${LLVM_LINK_OR_COPY} "${SPIRV_VAL}" "${LLVM_RUNTIME_OUTPUT_INTDIR}/spirv-val") +else () + add_custom_target(spirv-val + COMMAND ${CMAKE_COMMAND} -E ${LLVM_LINK_OR_COPY} "${BINARY_DIR}/tools/spirv-val" "${LLVM_RUNTIME_OUTPUT_INTDIR}/spirv-val" + DEPENDS SPIRVTools + ) +endif () 
@sudonatalie
Copy link
Member Author

@Keenuts @michalpaszkowski I'll continue to monitor the RFC for any more incoming feedback but for now I think it's reasonable to go ahead with review on this, so please take a look whenever you can.

Copy link
Contributor

@Keenuts Keenuts left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together. I'd like to have this PR in 😊

Add spirv-dis (disassembler) and spirv-val (validator) from SPIRV-Tools as external dependencies for testing the SPIR-V backend. These tools are test dependencies only. SPIR-V tests now have a dependency on the spirv-dis and spirv-val targets, and are only built when the LLVM_INCLUDE_SPIRV_TOOLS_TESTS cmake variable is set. Two tests are included to show these tools' use, however more tests will be migrated and added later. * OpVariable_order.ll shows how spirv-val can catch bugs in the backend. * basic_int_types.ll shows how tests can be much shorter and more readable by FileChecking the spirv-dis output.
@sudonatalie sudonatalie force-pushed the add-spirv-tools branch 3 times, most recently from f8af8c5 to cb04f7d Compare December 1, 2023 17:20
@sudonatalie
Copy link
Member Author

I see the bot failure, but I'm not yet sure why @LLVM_INCLUDE_SPIRV_TOOLS_TESTS@ is being substituted with OFF while other cmake vars in the file are being substituted with 0. Looking into it.

@sudonatalie sudonatalie merged commit f368e64 into llvm:main Dec 4, 2023
@sudonatalie sudonatalie deleted the add-spirv-tools branch December 4, 2023 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

4 participants