KhronosGroup / KhronosGroup/SPIRV-Tools

[spirv-opt] OpLoads are not de-duplicated for NonWritable buffers

Open
#6,412 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
1.4k
Forks
709
Avg merge
1d 22h
Merged PRs (30d)
28

Description

It's pretty common to loads parameters that are stored in buffers, with a simplified test case being:
```hlsl
StructuredBuffer in_data;
RWStructuredBuffer data_out;

[numthreads(1, 1, 1)]
void test_0(uint index : SV_DispatchThreadID)
{
data_out[0] = in_data[0];
data_out[1] = in_data[0];
}
```

With `-O -Os` we get:
```llvm
; SPIR-V
; Version: 1.0
; Generator: Google spiregg; 0
; Bound: 24
; Schema: 0
OpCapability Shader
OpMemoryModel Logical GLSL450
OpEntryPoint GLCompute %1 "test_0"
OpExecutionMode %1 LocalSize 1 1 1
OpDecorate %3 DescriptorSet 0
OpDecorate %3 Binding 1
OpDecorate %5 DescriptorSet 0
OpDecorate %5 Binding 2
OpDecorate %_runtimearr_float ArrayStride 4
OpMemberDecorate %_struct_2 0 Offset 0
OpMemberDecorate %_struct_2 0 NonWritable
OpDecorate %_struct_2 BufferBlock
OpMemberDecorate %_struct_4 0 Offset 0
OpDecorate %_struct_4 BufferBlock
%int = OpTypeInt 32 1
%int_0 = OpConstant %int 0
%uint = OpTypeInt 32 0
%uint_0 = OpConstant %uint 0
%uint_1 = OpConstant %uint 1
%float = OpTypeFloat 32
%_runtimearr_float = OpTypeRuntimeArray %float
%_struct_2 = OpTypeStruct %_runtimearr_float
%_ptr_Uniform__struct_2 = OpTypePointer Uniform %_struct_2
%_struct_4 = OpTypeStruct %_runtimearr_float
%_ptr_Uniform__struct_4 = OpTypePointer Uniform %_struct_4
%void = OpTypeVoid
%16 = OpTypeFunction %void
%_ptr_Uniform_float = OpTypePointer Uniform %float
%3 = OpVariable %_ptr_Uniform__struct_2 Uniform
%5 = OpVariable %_ptr_Uniform__struct_4 Uniform
%1 = OpFunction %void None %16
%18 = OpLabel
%19 = OpAccessChain %_ptr_Uniform_float %3 %int_0 %uint_0
%20 = OpLoad %float %19
%21 = OpAccessChain %_ptr_Uniform_float %5 %int_0 %uint_0
OpStore %21 %20
%22 = OpLoad %float %19
%23 = OpAccessChain %_ptr_Uniform_float %5 %int_0 %uint_1
OpStore %23 %22
OpReturn
OpFunctionEnd
```

With the following duplicate loads:
```
%20 = OpLoad %float %19
%22 = OpLoad %float %19
```

Despite:
```llvm
OpMemberDecorate %_struct_2 0 Offset 0
OpMemberDecorate %_struct_2 0 NonWritable
%3 = OpVariable %_ptr_Uniform__struct_2 Uniform
%19 = OpAccessChain %_ptr_Uniform_float %3 %int_0 %uint_0
```

I believe, this is because the use of `bool Instruction::IsReadOnlyPointerShaders()` is a bit off, while it does check for `NonWritable`:
```cpp
bool is_nonwritable = false;
context()->get_decoration_mgr()->ForEachDecoration(
result_id(), uint32_t(spv::Decoration::NonWritable),
[&is_nonwritable](const Instruction&) { is_nonwritable = true; });
return is_nonwritable;
```

It's testing against `%3`, which is the `OpVariable`, which would only have decorators for the binding set and descriptor.

Contributor guide

Open the contributing guide

Research direction

Start at Instruction::IsReadOnlyPointerShaders() and trace how it checks NonWritable decorations on the pointer used by the OpLoad. Reproduce the HLSL example with spirv-opt -O -Os, then verify that repeated loads from the NonWritable buffer are de-duplicated and the generated SPIR-V contains one OpLoad.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.