[Feature Request] [SPIRV] Support `SPV_EXT_shader_invocation_reorder`

[MarijnS95]

Is your feature request related to a problem? Please describe.

#6994 for the older SPV_NV_shader_invocation_reorder was previously closed as blocking on microsoft/hlsl-specs#277 to add a SER proposal to HLSL first. Now that that's in, and the SPIR-V extension was promoted to EXT, would it be possible to target SPV_EXT_shader_invocation_reorder in SPIR-V?

Describe the solution you'd like
When dx::HitObject is used in code that is compiled to SPIR-V with dxc -spirv, the SPV_EXT_shader_invocation_reorder extension is used.

Describe alternatives you've considered
The Vulkan VK_NV_ray_tracing_invocation_reorder extension mentions how to use SPIR-V intrinsics to achieve this, which should trivially translate to SPV_EXT_shader_invocation_reorder even if a manual conditional implementation needs to be added next to existing dx::HitObject usage.

Additional context
Add any other context or screenshots about the feature request here.

[MarijnS95]

EDIT: I've actually got a sucessful SER implementation going now. At first the Nvidia driver/device I was trying on didn't like HitObjectEXT being a struct member of a generic struct HitObject { HitObjectEXT inner; functions()... }; that I crafted to abstract both custom SPIR-V and the existing dx::HitObject but when either making that a global static or inlining it in functions it started working. Here I also used some GLSL samples to doublecheck that my custom SPIR-V was correct, together with updated spirv-val 🎉

---

Fwiw https://www.khronos.org/blog/boosting-ray-tracing-performance-with-shader-execution-reordering-introducing-vk-ext-ray-tracing-invocation-reorder mentions:

Hit objects and SER are also available in HLSL 6.9. If you’re generating SPIR-V using HLSL input, then as of this writing you’ll need to use HLSL SPIR-V intrinsics. For information and examples, please see the HLSL Mapping section of the VK_EXT_ray_tracing_invocation_reorder specification.

(The following is likely feedback for the Khronos blog, not DXC)

This for one links the deprecated page instead of https://github.com/microsoft/DirectXShaderCompiler/wiki/Inline-SPIR%E2%80%90V (slight differences in type definitions it seems).

---

Secondly the spec page doesn't mention a HLSL mapping, but the feature "proposal" page does:

https://docs.vulkan.org/refpages/latest/refpages/source/VK_EXT_ray_tracing_invocation_reorder.html
https://docs.vulkan.org/features/latest/features/proposals/VK_EXT_ray_tracing_invocation_reorder.html

At least the alleged:

[[vk::ext_type_def(HitObjectAttributeEXT, OpTypeHitObjectEXT)]]
void createHitObjectEXT();
#define HitObjectEXT vk::ext_type<HitObjectAttributeEXT>

With a required call to createHitObjectEXT() to ensure the type becomes OpTypeHitObjectEXT is no longer necessary with replacing it all with a single:

using HitObjectEXT = vk::SpirvOpaqueType<OpTypeHitObjectEXT>;

🎉

---

Additionally the use of HitObjectAttributeEXT in this position is nothing short of confusing. vk::ext_type+vk::ext_type_def defines that argument as:

uint unique_type_id can be any constant unsigned integer number, but it must be a unique identifier for the defined type. It will be used by vk::ext_type

So it specifically being HitObjectAttributeEXT here has no significance anyway and is mostly just confusing, because that attribute number is used to annotate the storage class of geometry attributes instead.

---

Finally I'm seeing that HitObjectEXT h; is unused when its reference is passed to hitObjectTraceRayEXT() (or hitObjectRecordEmptyEXT() for that matter), both of which should initialize it. What's the best way to annotate that it is an "out" reference?

<source>:72:26: warning: variable 'h' is uninitialized when used here [-Wuninitialized]
    hitObjectTraceRayEXT(h, tlas, flags, 0xff, 0, 0, 0,
                         ^
<source>:71:5: note: variable 'h' is declared here
    HitObjectEXT h;
    ^

[MarijnS95]

Regarding this:

At first the Nvidia driver/device I was trying on didn't like HitObjectEXT being a struct member of a generic struct HitObject { HitObjectEXT inner; functions()... }; that I crafted to abstract both custom SPIR-V and the existing dx::HitObject but when either making that a global static or inlining it in functions it started working.

Is the resulting OpAccessChain to pull the hitObjectEXT out of my HitObject abstraction actually valid?

[devshgraphicsprogramming]

The HitObjectAttributeEXT storage class is for passing AABB intersection hit shaders attributes back to raygen via hitObjectGetAttributesEXT not for storing the HitObject itself I think

[MarijnS95]

Exactly, that's why it's all the more confusing that that "arbitrary integer" is used for this purpose. Hence I'm a bigger fan of just using:

using HitObjectEXT = vk::SpirvOpaqueType<OpTypeHitObjectEXT>;

Which doesn't require an arbitrary extra identifier at all.

[devshgraphicsprogramming]

Exactly, that's why it's all the more confusing that that "arbitrary integer" is used for this purpose. Hence I'm a bigger fan of just using:

using HitObjectEXT = vk::SpirvOpaqueType;

Which doesn't require an arbitrary extra identifier at all.

the docs are completely wrong and wont work/compile

They also use ancient experimental inline SPIR-V syntax, not the new tried and tested stuff from about 24-18 months ago

I should have a working SER example with inline SPIR-V in Nabla STL today which you can copy & paste if you respect the Apache 2.0 license.

In the meantime this is the correct and only usage of the HitObjectAttributeEXT storage class

            [[vk::ext_storage_class(spv::StorageClassHitObjectAttributeEXT)]] float32_t2 tmp;
            spirv::hitObjectGetAttributesEXT(hitObject,tmp);
            retval.barycentrics = tmp;

[devshgraphicsprogramming]

I'm actually having a huge problem exposing OpHitObjectGetIntersectionTriangleVertexPositionsEXT because HLSL is weird

// the syntax to declare a function returning an Array in HLSL is insane
[[vk::ext_capability(spv::CapabilityShaderInvocationReorderEXT)]]
[[vk::ext_extension("SPV_EXT_shader_invocation_reorder")]]
[[vk::ext_instruction(spv::OpHitObjectGetIntersectionTriangleVertexPositionsEXT)]]
[[vk::ext_capability(spv::CapabilityRayQueryPositionFetchKHR)]]
[[vk::ext_extension("SPV_KHR_ray_tracing_position_fetch")]]
float32_t3 hitObjectGetIntersectionTriangleVertexPositionsEXT([[vk::ext_reference]] HitObjectEXT hitObject)[3];

[MarijnS95]

We already have a fully working and tested abstraction using the new inline SPIR-V syntax, since requesting this built-in abstraction a month ago. Nothing that we need to take from another project. It's just a bummer that both documentation pages (including the EXT proposal) use the legacy syntax.