Unnecissary VariablePointers Capability Added

[cwfitzgerald]

I'm not sure exactly whose issue this is, so I'm cross-filing it here and on spirv tools. When I use make any program with a pointer in it, the VariablePointers capability is enabled, even if it is not required. This isn't a big deal with shipping, as we're shipping on vk 1.2, but Renderdoc refuses to debug shaders with the VariablePointer capability, even if unused.

I filed KhronosGroup/SPIRV-Tools#5854 against spirv-tools because their "trim-capabilities" pass wasn't removing it either.

My interim solution has been to disassemble, remove the cap, then re-assemble and re-validate, which has been working for all of my uses of BDA.

For example:

uniform uint *value;

[shader("compute")]
void cs()
{
    *value = 42;
}

Generates:

; SPIR-V
; Version: 1.5
; Generator: Khronos; 40
; Bound: 18
; Schema: 0
OpCapability VariablePointers
OpCapability PhysicalStorageBufferAddresses
OpCapability Shader
OpExtension "SPV_KHR_variable_pointers"
OpExtension "SPV_KHR_physical_storage_buffer"
OpMemoryModel PhysicalStorageBuffer64 GLSL450
OpEntryPoint GLCompute %cs "main" %globalParams
OpExecutionMode %cs LocalSize 1 1 1

; Debug Information
OpSource Slang 1
OpName %GlobalParams_std140 "GlobalParams_std140"   ; id %5
OpMemberName %GlobalParams_std140 0 "value"
OpName %globalParams "globalParams"                 ; id %9
OpName %cs "cs"                                     ; id %2

; Annotations
OpDecorate %_ptr_PhysicalStorageBuffer_uint ArrayStride 4
OpDecorate %GlobalParams_std140 Block
OpMemberDecorate %GlobalParams_std140 0 Offset 0
OpDecorate %globalParams Binding 0
OpDecorate %globalParams DescriptorSet 0

; Types, variables and constants
%void = OpTypeVoid
%3 = OpTypeFunction %void
%uint = OpTypeInt 32 0
%_ptr_PhysicalStorageBuffer_uint = OpTypePointer PhysicalStorageBuffer %uint    ; ArrayStride 4
%GlobalParams_std140 = OpTypeStruct %_ptr_PhysicalStorageBuffer_uint            ; Block
%_ptr_Uniform_GlobalParams_std140 = OpTypePointer Uniform %GlobalParams_std140
%int = OpTypeInt 32 1
%int_0 = OpConstant %int 0
%_ptr_Uniform__ptr_PhysicalStorageBuffer_uint = OpTypePointer Uniform %_ptr_PhysicalStorageBuffer_uint
%uint_42 = OpConstant %uint 42
%globalParams = OpVariable %_ptr_Uniform_GlobalParams_std140 Uniform    ; Binding 0, DescriptorSet 0

; Function cs
%cs = OpFunction %void None %3
%4 = OpLabel
%13 = OpAccessChain %_ptr_Uniform__ptr_PhysicalStorageBuffer_uint %globalParams %int_0
%14 = OpLoad %_ptr_PhysicalStorageBuffer_uint %13
OpStore %14 %uint_42 Aligned 4
OpReturn
OpFunctionEnd

If I take this spirv and remove the OpCapability VariablePointers and OpExtension "SPV_KHR_variable_pointers" from the spirv and re-assemble and validate, it passes.

[tuxerr]

I was gonna file this issue so thanks for doing it ;)

FWIW It also breaks SPIRV-Cross both decompiling to GLSL and HLSL, so it's a pretty big issue.

I get this problem both when using direct pointers, but also when using ConstBufferPointer<> as a replacement for vk::RawBufferLoad.

    ConstBufferPointer<MeshletEntry> meshlets;
    DrawBuffer *drawbuf;

[csyonghe]

After reviewing the spirv spec, I think VariablePointer capability is not required, but we do need VariablePointerStroageBuffer to be able to do anything meaningful. I opened PR 5340 to attempt to switch to VariablePointerStroageBuffer, will see what happens in the CI.

[cwfitzgerald]

Looking at the context of the pr, it has a check right before to see if the addressing mode of the memory model is PhysicalStorageBuffer64 and skipping the variable pointer caps if it is. The output spirv does have that addressing mode enabled, which should be able to cover most normal usages of BDA.

[csyonghe]

Well, that if is just to prevent redundantly emitting the decoration. The behavior is that as long as you have pointers, we need to declare VariablePointerStorageBuffer.

[csyonghe]

There maybe more we can do, but without it we cannot meaningfully use pointers in any way, including using the access chain instruction.

The right solve IMO is to make the debugging tools support pointers.

[tuxerr]

FWIW that won't solve the SPIRV-Cross issue as GLSL and HLSL inherently don't support pointers (and decompiling to those languages is quite useful). Given that here we're doing things in slang that ARE supported in GLSL and HLSL (basic buffer_device_address usage), it'd be a shame to drop compatibility with them when the shader doesn't make use of that capability.

[csyonghe]

OK, so you are saying all you did with the shader is directly loading a value from the pointer, and you want variable pointers to not be defined in that case. We can certainly do that, yeah.

[csyonghe]

and just to be clear, if the shader does things like ptr->member or ptr[x] or Int* v = ptr;, then we cannot avoid this extension. Does that sound reasonable?

[cwfitzgerald]

Reading through the spec for physical storage buffer, VariablePointers shouldn't be needed for doing those things with BDA pointers. As VariablePointers works on only logical pointers, but the spec defines all pointers into the physical storage buffer memory type as physical (not logical), so you should just be able to access them fine. Spirv-val seems to agree with this. OpPtrAccessChain should be supported when accessing a physical buffer pointer without variable pointers.

[csyonghe]

Hmm, perhaps I misread the spec document. I am going to try completely removing the declaration and see if things work just fine.

[csyonghe]

@cwfitzgerald You are right that this capability is not required. I have removed it in the PR.

[cwfitzgerald]

Nice, thanks!