KhronosGroup / SPIRV-Tools

Apache License 2.0
1.06k stars 549 forks source link

Raytracing extension/capability not removed when `RaytracingAccelerationStructure` is deadstripped #5358

Closed MarijnS95 closed 1 year ago

MarijnS95 commented 1 year ago

Hi! We rely on a bindless model where we have one HLSL "header" that defines all our bindings, access wrappers for these, and various DXIL/SPIR-V-specific bits to implement it. Unfortunately, as we expose ray-tracing bindings through this header, it currently means that all our shaders get the ray tracing extension and capability even if they don't use any ray tracing operations, and the RaytracingAccelerationStructure binding ultimately gets culled.

As far as I understand/remember it is up to spirv-opt's DCE to get rid of the unreferenced TLAS binding. DXC originally emits the extension and capability when it sees the TLAS type (with a note for needing another spirv-opt pass when the target is a ray_pipeline and not a ray_query shader... not to mention ray_pipeline shaders containing extra inline ray_querys):

https://github.com/microsoft/DirectXShaderCompiler/blob/a5b0488bc5b20659662bdfdad134c13cb376189b/tools/clang/lib/SPIRV/CapabilityVisitor.cpp#L217-L228

There is also a test in DXC to assert that the capability is not culled even if the binding is not used:

https://github.com/microsoft/DirectXShaderCompiler/blob/main/tools/clang/test/CodeGenSPIRV/raytracing.acceleration-structure.hlsl

We observe the same with a simple example shader:

struct VsOut {
    uint someIndex : TEXCOORD1;
};

[[vk::binding(0, 1)]] RaytracingAccelerationStructure g_tlas;

VsOut main(uint vertexIndex : SV_VertexID) {
    VsOut vOut = (VsOut)0;
    vOut.someIndex = 1234;

    return vOut;
}

Compile the above with dxc -E main -T vs_6_6 rt-dce.hlsl -spirv:

; SPIR-V
; Version: 1.0
; Generator: Google spiregg; 0
; Bound: 9
; Schema: 0
               OpCapability Shader
               OpCapability RayQueryKHR
               OpExtension "SPV_KHR_ray_query"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Vertex %main "main" %out_var_TEXCOORD1
               OpSource HLSL 660
               OpName %out_var_TEXCOORD1 "out.var.TEXCOORD1"
               OpName %main "main"
               OpDecorate %out_var_TEXCOORD1 Flat
               OpDecorate %out_var_TEXCOORD1 Location 0
       %uint = OpTypeInt 32 0
  %uint_1234 = OpConstant %uint 1234
%_ptr_Output_uint = OpTypePointer Output %uint
       %void = OpTypeVoid
          %7 = OpTypeFunction %void
%out_var_TEXCOORD1 = OpVariable %_ptr_Output_uint Output
       %main = OpFunction %void None %7
          %8 = OpLabel
               OpStore %out_var_TEXCOORD1 %uint_1234
               OpReturn
               OpFunctionEnd

Observe OpCapability RayQueryKHR and OpExtension "SPV_KHR_ray_query" even though there are no ray-tracing operations nor bindings in this shader anymore.

To confirm that it has been spirv-opt that removed the binding, according to https://github.com/microsoft/DirectXShaderCompiler/blob/main/docs/SPIR-V.rst#optimization all optimization including DCE is offloaded to spirv-opt, which we can turn off using -fcgl and observe that the TLAS binding is there, emitted by DXC despite being unused:

               OpCapability Shader
               OpCapability RayQueryKHR
               OpExtension "SPV_KHR_ray_query"
...
               OpName %accelerationStructureNV "accelerationStructureNV"
               OpName %g_tlas "g_tlas"
...
               OpDecorate %g_tlas DescriptorSet 1
               OpDecorate %g_tlas Binding 0
...
%accelerationStructureNV = OpTypeAccelerationStructureKHR
%_ptr_UniformConstant_accelerationStructureNV = OpTypePointer UniformConstant %accelerationStructureNV
...
     %g_tlas = OpVariable %_ptr_UniformConstant_accelerationStructureNV UniformConstant
...

I am not sure where this is supposed to be fixed, as it sounds like a chicken-and-egg problem that has many different solutions. Maybe a post-pass checking if a certain extension/capability is still in use by any of the other Ops? This is needed anyway if the DCE pass were to try and understand that it just removed an Op which requires an ext/cap... and can only remove the ext/cap when there are no other Ops still requiring it which must become wildly complex and error-prone.

s-perron commented 1 year ago

@Keenuts is actively working on a pass that will remove unused capabilities. The skeleton with some basic functionality. We are working to integrate it into DXC. After that we will improve the pass to remove more capabilities.

MarijnS95 commented 1 year ago

@s-perron that's lovely to hear, didn't expect this to be picked up so quickly!