Skip to content
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.

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

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

Closed
MarijnS95 opened this issue Aug 3, 2023 · 2 comments · Fixed by #5397

Comments

@MarijnS95
Copy link
Contributor

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
Copy link
Collaborator

s-perron commented Aug 8, 2023

@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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants