Skip to content

[HLSL] Fix failing SPIR-V backend tests that specify --target-env vulkan1.3 #136344

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

Jump to bottom
Closed
kmpeng opened this issue Apr 18, 2025 · 2 comments · Fixed by #137633
Closed

[HLSL] Fix failing SPIR-V backend tests that specify --target-env vulkan1.3 #136344

kmpeng opened this issue Apr 18, 2025 · 2 comments · Fixed by #137633
Assignees
@s-perron
Labels
backend:SPIR-V HLSL HLSL Language Support test-suite

Comments

@kmpeng
Copy link
Contributor

kmpeng commented Apr 18, 2025 •
edited
Loading

Problem

When SPIRV-Tools is enabled, tests that specify the target environment vulkan1.3 in the validation step fail. The tests are:

  • CodeGen/SPIRV/hlsl-intrinsics/SV_GroupIndex.ll (link)
  • CodeGen/SPIRV/hlsl-intrinsics/smoothstep.ll (link)

Temporary fix

This PR was a stop gap to get the pipeline green again:
#136343

Long term fix

The current suggestion from @s-perron is to change the tests to not have external functions or variables, e.g. making the functions shader entry points. More discussion is likely needed.
@EugeneZelenko EugeneZelenko added test-suite HLSL HLSL Language Support and removed new issue labels Apr 18, 2025
@kmpeng kmpeng mentioned this issue Apr 18, 2025
Merged
@s-perron s-perron self-assigned this Apr 18, 2025
kmpeng added a commit that referenced this issue Apr 18, 2025
@kmpeng
[HLSL] XFAIL smoothstep.ll and SV_GroupIndex.ll SPIR-V backend te…
a79e257 
…sts (#136343)

When SPIRV-Tools is enabled, tests that specify the target environment
`vulkan1.3` in the validation step fail. This is because SPIRV-Tools
seems to have [grown some capability checks for vulkan
1.3](KhronosGroup/SPIRV-Tools@7e41c71).
The failing tests are:
- `CodeGen/SPIRV/hlsl-intrinsics/SV_GroupIndex.ll`
- `CodeGen/SPIRV/hlsl-intrinsics/smoothstep.ll`

For now, the fix is to XFAIL these tests to unblock the pipeline.

Issue #136344 tracks the long-term solution for this.
This was referenced Apr 18, 2025
Closed
Merged
@llvm-beanz
Copy link
Collaborator

I've advised @kmpeng to have new SPIR-V tests use --target-env spv1.4 instead of a Vulkan target since that works.

@s-perron we should figure out how we want these tests written for Vulkan and we can circle back to update the test cases.

@s-perron
Copy link
Contributor

Sure. However, we need should try to have the API we are trying to target in the spirv-val step. Many details are left to the target environment. The "universal" environment will simply not check these requirements.

For example, the LocalInvocationIndex in SPIR-V says:

Local invocation index in GLCompute Execution Models. See the client API specification for more detail.

Then you need to know that it is vulkan for the validator to check for API specific requirements:
https://docs.vulkan.org/spec/latest/chapters/interfaces.html#VUID-LocalInvocationIndex-LocalInvocationIndex-04284

The validator may not check everything yet, but it is good to catch it where we can.

kmpeng added a commit that referenced this issue Apr 24, 2025
@kmpeng
[HLSL] Implement the faceforward intrinsic (#135878)
67cbfb9 
Resolves #99114.

Tasks completed:
- Implement `faceforward` in
`hlsl_intrinsics.h`/`hlsl_intrinsic_helpers.h`
- Implement `faceforward` SPIR-V target builtin in
`clang/include/clang/Basic/BuiltinsSPIRV.td`
- Add a SPIR-V fast path in `hlsl_intrinsic_helpers.h`
- Add sema checks for `faceforward` to `CheckSPIRVBuiltinFunctionCall`
in `clang/lib/Sema/SemaSPIRV.cpp`
- Add codegen for SPIR-V `faceforward` builtin to `EmitSPIRVBuiltinExpr`
in `SPIR.cpp`
- Add HLSL codegen tests to
`clang/test/CodeGenHLSL/builtins/faceforward.hlsl`
- Add SPIRV builtin codegen tests to
`clang/test/CodeGenSPIRV/Builtins/faceforward.c`
- Add sema tests to
`clang/test/SemaHLSL/BuiltIns/faceforward-errors.hlsl`
- Add spirv sema tests to
`clang/test/SemaSPIRV/BuiltIns/faceforward-errors.c`
- Create the `int_spv_faceforward` intrinsic in `IntrinsicsSPIRV.td`
- In `SPIRVInstructionSelector.cpp` create the `faceforward` lowering
and map it to `int_spv_faceforward` in
`SPIRVInstructionSelector::selectIntrinsic`
- Create SPIR-V backend test case in
`llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll`

Incomplete tasks:
- Create SPIR-V backend test case in
`llvm/test/CodeGen/SPIRV/opencl/faceforward.ll`
- Not applicable because the OpenCL SPIR-V extended instruction set does
not include a `faceforward` function

Follow-up tasks:
- Implement pattern matching for `faceforward` in `SPIRVCombine.td` and
`SPIRVPreLegalizerCombiner.cpp`
- In `faceforward.ll`, change `--target-env spv1.4` to `vulkan1.3` and
update the test accordingly once
[#136344](#136344) has been
resolved
@s-perron s-perron moved this to Active in HLSL Support Apr 28, 2025
@s-perron s-perron mentioned this issue Apr 28, 2025
Merged
s-perron added a commit to s-perron/llvm-project that referenced this issue Apr 28, 2025
@s-perron
[SPIRV] Fix test with VK1.4 validation
c6b5439 
An update to the spirv validator is now correctly rejecting vulkan
shaders with the linkage capability. We have a couple tests that need
updating to remove the capability.

Fixes llvm#136344
s-perron added a commit that referenced this issue Apr 30, 2025
@s-perron
[SPIRV] Fix test with VK1.3 validation (#137633)
a5fef2a 
An update to the spirv validator is now correctly rejecting vulkan
shaders with the linkage capability. We have a couple tests that need
updating to remove the capability.

Fixes #136344
@github-project-automation github-project-automation bot moved this from Active to Closed in HLSL Support Apr 30, 2025
charles-zablit pushed a commit to charles-zablit/llvm-project that referenced this issue May 1, 2025
@s-perron @charles-zablit
[SPIRV] Fix test with VK1.3 validation (llvm#137633)
8034a6d 
An update to the spirv validator is now correctly rejecting vulkan
shaders with the linkage capability. We have a couple tests that need
updating to remove the capability.

Fixes llvm#136344
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this issue May 6, 2025
@kmpeng @github-actions
Automerge: [HLSL] Implement the faceforward intrinsic (#135878)
85b7f54 
Resolves #99114.

Tasks completed:
- Implement `faceforward` in
`hlsl_intrinsics.h`/`hlsl_intrinsic_helpers.h`
- Implement `faceforward` SPIR-V target builtin in
`clang/include/clang/Basic/BuiltinsSPIRV.td`
- Add a SPIR-V fast path in `hlsl_intrinsic_helpers.h`
- Add sema checks for `faceforward` to `CheckSPIRVBuiltinFunctionCall`
in `clang/lib/Sema/SemaSPIRV.cpp`
- Add codegen for SPIR-V `faceforward` builtin to `EmitSPIRVBuiltinExpr`
in `SPIR.cpp`
- Add HLSL codegen tests to
`clang/test/CodeGenHLSL/builtins/faceforward.hlsl`
- Add SPIRV builtin codegen tests to
`clang/test/CodeGenSPIRV/Builtins/faceforward.c`
- Add sema tests to
`clang/test/SemaHLSL/BuiltIns/faceforward-errors.hlsl`
- Add spirv sema tests to
`clang/test/SemaSPIRV/BuiltIns/faceforward-errors.c`
- Create the `int_spv_faceforward` intrinsic in `IntrinsicsSPIRV.td`
- In `SPIRVInstructionSelector.cpp` create the `faceforward` lowering
and map it to `int_spv_faceforward` in
`SPIRVInstructionSelector::selectIntrinsic`
- Create SPIR-V backend test case in
`llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll`

Incomplete tasks:
- Create SPIR-V backend test case in
`llvm/test/CodeGen/SPIRV/opencl/faceforward.ll`
- Not applicable because the OpenCL SPIR-V extended instruction set does
not include a `faceforward` function

Follow-up tasks:
- Implement pattern matching for `faceforward` in `SPIRVCombine.td` and
`SPIRVPreLegalizerCombiner.cpp`
- In `faceforward.ll`, change `--target-env spv1.4` to `vulkan1.3` and
update the test accordingly once
[#136344](llvm/llvm-project#136344) has been
resolved
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this issue May 6, 2025
@kmpeng @IanWood1
[HLSL] XFAIL smoothstep.ll and SV_GroupIndex.ll SPIR-V backend te…
dc6efa8 
…sts (llvm#136343)

When SPIRV-Tools is enabled, tests that specify the target environment
`vulkan1.3` in the validation step fail. This is because SPIRV-Tools
seems to have [grown some capability checks for vulkan
1.3](KhronosGroup/SPIRV-Tools@7e41c71).
The failing tests are:
- `CodeGen/SPIRV/hlsl-intrinsics/SV_GroupIndex.ll`
- `CodeGen/SPIRV/hlsl-intrinsics/smoothstep.ll`

For now, the fix is to XFAIL these tests to unblock the pipeline.

Issue llvm#136344 tracks the long-term solution for this.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this issue May 6, 2025
@kmpeng @IanWood1
[HLSL] Implement the faceforward intrinsic (llvm#135878)
a972d30 
Resolves llvm#99114.

Tasks completed:
- Implement `faceforward` in
`hlsl_intrinsics.h`/`hlsl_intrinsic_helpers.h`
- Implement `faceforward` SPIR-V target builtin in
`clang/include/clang/Basic/BuiltinsSPIRV.td`
- Add a SPIR-V fast path in `hlsl_intrinsic_helpers.h`
- Add sema checks for `faceforward` to `CheckSPIRVBuiltinFunctionCall`
in `clang/lib/Sema/SemaSPIRV.cpp`
- Add codegen for SPIR-V `faceforward` builtin to `EmitSPIRVBuiltinExpr`
in `SPIR.cpp`
- Add HLSL codegen tests to
`clang/test/CodeGenHLSL/builtins/faceforward.hlsl`
- Add SPIRV builtin codegen tests to
`clang/test/CodeGenSPIRV/Builtins/faceforward.c`
- Add sema tests to
`clang/test/SemaHLSL/BuiltIns/faceforward-errors.hlsl`
- Add spirv sema tests to
`clang/test/SemaSPIRV/BuiltIns/faceforward-errors.c`
- Create the `int_spv_faceforward` intrinsic in `IntrinsicsSPIRV.td`
- In `SPIRVInstructionSelector.cpp` create the `faceforward` lowering
and map it to `int_spv_faceforward` in
`SPIRVInstructionSelector::selectIntrinsic`
- Create SPIR-V backend test case in
`llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll`

Incomplete tasks:
- Create SPIR-V backend test case in
`llvm/test/CodeGen/SPIRV/opencl/faceforward.ll`
- Not applicable because the OpenCL SPIR-V extended instruction set does
not include a `faceforward` function

Follow-up tasks:
- Implement pattern matching for `faceforward` in `SPIRVCombine.td` and
`SPIRVPreLegalizerCombiner.cpp`
- In `faceforward.ll`, change `--target-env spv1.4` to `vulkan1.3` and
update the test accordingly once
[llvm#136344](llvm#136344) has been
resolved
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this issue May 6, 2025
@s-perron @IanWood1
[SPIRV] Fix test with VK1.3 validation (llvm#137633)
8ebbfc7 
An update to the spirv validator is now correctly rejecting vulkan
shaders with the linkage capability. We have a couple tests that need
updating to remove the capability.

Fixes llvm#136344
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this issue May 6, 2025
@kmpeng @IanWood1
[HLSL] XFAIL smoothstep.ll and SV_GroupIndex.ll SPIR-V backend te…
3c22469 
…sts (llvm#136343)

When SPIRV-Tools is enabled, tests that specify the target environment
`vulkan1.3` in the validation step fail. This is because SPIRV-Tools
seems to have [grown some capability checks for vulkan
1.3](KhronosGroup/SPIRV-Tools@7e41c71).
The failing tests are:
- `CodeGen/SPIRV/hlsl-intrinsics/SV_GroupIndex.ll`
- `CodeGen/SPIRV/hlsl-intrinsics/smoothstep.ll`

For now, the fix is to XFAIL these tests to unblock the pipeline.

Issue llvm#136344 tracks the long-term solution for this.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this issue May 6, 2025
@kmpeng @IanWood1
[HLSL] Implement the faceforward intrinsic (llvm#135878)
c736834 
Resolves llvm#99114.

Tasks completed:
- Implement `faceforward` in
`hlsl_intrinsics.h`/`hlsl_intrinsic_helpers.h`
- Implement `faceforward` SPIR-V target builtin in
`clang/include/clang/Basic/BuiltinsSPIRV.td`
- Add a SPIR-V fast path in `hlsl_intrinsic_helpers.h`
- Add sema checks for `faceforward` to `CheckSPIRVBuiltinFunctionCall`
in `clang/lib/Sema/SemaSPIRV.cpp`
- Add codegen for SPIR-V `faceforward` builtin to `EmitSPIRVBuiltinExpr`
in `SPIR.cpp`
- Add HLSL codegen tests to
`clang/test/CodeGenHLSL/builtins/faceforward.hlsl`
- Add SPIRV builtin codegen tests to
`clang/test/CodeGenSPIRV/Builtins/faceforward.c`
- Add sema tests to
`clang/test/SemaHLSL/BuiltIns/faceforward-errors.hlsl`
- Add spirv sema tests to
`clang/test/SemaSPIRV/BuiltIns/faceforward-errors.c`
- Create the `int_spv_faceforward` intrinsic in `IntrinsicsSPIRV.td`
- In `SPIRVInstructionSelector.cpp` create the `faceforward` lowering
and map it to `int_spv_faceforward` in
`SPIRVInstructionSelector::selectIntrinsic`
- Create SPIR-V backend test case in
`llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll`

Incomplete tasks:
- Create SPIR-V backend test case in
`llvm/test/CodeGen/SPIRV/opencl/faceforward.ll`
- Not applicable because the OpenCL SPIR-V extended instruction set does
not include a `faceforward` function

Follow-up tasks:
- Implement pattern matching for `faceforward` in `SPIRVCombine.td` and
`SPIRVPreLegalizerCombiner.cpp`
- In `faceforward.ll`, change `--target-env spv1.4` to `vulkan1.3` and
update the test accordingly once
[llvm#136344](llvm#136344) has been
resolved
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this issue May 6, 2025
@s-perron @IanWood1
[SPIRV] Fix test with VK1.3 validation (llvm#137633)
a7f2d06 
An update to the spirv validator is now correctly rejecting vulkan
shaders with the linkage capability. We have a couple tests that need
updating to remove the capability.

Fixes llvm#136344
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this issue May 6, 2025
@kmpeng @IanWood1
[HLSL] XFAIL smoothstep.ll and SV_GroupIndex.ll SPIR-V backend te…
bfa885b 
…sts (llvm#136343)

When SPIRV-Tools is enabled, tests that specify the target environment
`vulkan1.3` in the validation step fail. This is because SPIRV-Tools
seems to have [grown some capability checks for vulkan
1.3](KhronosGroup/SPIRV-Tools@7e41c71).
The failing tests are:
- `CodeGen/SPIRV/hlsl-intrinsics/SV_GroupIndex.ll`
- `CodeGen/SPIRV/hlsl-intrinsics/smoothstep.ll`

For now, the fix is to XFAIL these tests to unblock the pipeline.

Issue llvm#136344 tracks the long-term solution for this.
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this issue May 6, 2025
@kmpeng @IanWood1
[HLSL] Implement the faceforward intrinsic (llvm#135878)
7fe119a 
Resolves llvm#99114.

Tasks completed:
- Implement `faceforward` in
`hlsl_intrinsics.h`/`hlsl_intrinsic_helpers.h`
- Implement `faceforward` SPIR-V target builtin in
`clang/include/clang/Basic/BuiltinsSPIRV.td`
- Add a SPIR-V fast path in `hlsl_intrinsic_helpers.h`
- Add sema checks for `faceforward` to `CheckSPIRVBuiltinFunctionCall`
in `clang/lib/Sema/SemaSPIRV.cpp`
- Add codegen for SPIR-V `faceforward` builtin to `EmitSPIRVBuiltinExpr`
in `SPIR.cpp`
- Add HLSL codegen tests to
`clang/test/CodeGenHLSL/builtins/faceforward.hlsl`
- Add SPIRV builtin codegen tests to
`clang/test/CodeGenSPIRV/Builtins/faceforward.c`
- Add sema tests to
`clang/test/SemaHLSL/BuiltIns/faceforward-errors.hlsl`
- Add spirv sema tests to
`clang/test/SemaSPIRV/BuiltIns/faceforward-errors.c`
- Create the `int_spv_faceforward` intrinsic in `IntrinsicsSPIRV.td`
- In `SPIRVInstructionSelector.cpp` create the `faceforward` lowering
and map it to `int_spv_faceforward` in
`SPIRVInstructionSelector::selectIntrinsic`
- Create SPIR-V backend test case in
`llvm/test/CodeGen/SPIRV/hlsl-intrinsics/faceforward.ll`

Incomplete tasks:
- Create SPIR-V backend test case in
`llvm/test/CodeGen/SPIRV/opencl/faceforward.ll`
- Not applicable because the OpenCL SPIR-V extended instruction set does
not include a `faceforward` function

Follow-up tasks:
- Implement pattern matching for `faceforward` in `SPIRVCombine.td` and
`SPIRVPreLegalizerCombiner.cpp`
- In `faceforward.ll`, change `--target-env spv1.4` to `vulkan1.3` and
update the test accordingly once
[llvm#136344](llvm#136344) has been
resolved
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this issue May 6, 2025
@s-perron @IanWood1
[SPIRV] Fix test with VK1.3 validation (llvm#137633)
b6af7dc 
An update to the spirv validator is now correctly rejecting vulkan
shaders with the linkage capability. We have a couple tests that need
updating to remove the capability.

Fixes llvm#136344
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this issue May 6, 2025
@s-perron @github-actions
Automerge: [SPIRV] Fix test with VK1.3 validation (#137633)
a088744 
An update to the spirv validator is now correctly rejecting vulkan
shaders with the linkage capability. We have a couple tests that need
updating to remove the capability.

Fixes llvm/llvm-project#136344
GeorgeARM pushed a commit to GeorgeARM/llvm-project that referenced this issue May 7, 2025
@s-perron @GeorgeARM
[SPIRV] Fix test with VK1.3 validation (llvm#137633)
040d3f3 
An update to the spirv validator is now correctly rejecting vulkan
shaders with the linkage capability. We have a couple tests that need
updating to remove the capability.

Fixes llvm#136344
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@s-perron s-perron

Labels
backend:SPIR-V HLSL HLSL Language Support test-suite
Projects
HLSL Support
Status: Closed
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[SPIRV] Fix test with VK1.3 validation s-perron/llvm-project
5 participants
@EugeneZelenko @llvm-beanz @s-perron @kmpeng @llvmbot