KhronosGroup / SPIRV-Registry

SPIR-V specs
107 stars 71 forks source link

NonSemantic.Shader.DebugInfo.100 & forward references #203

Closed Keenuts closed 1 month ago

Keenuts commented 1 year ago

The NonSemantic.Shader.DebugInfo.100 has the following generic statement:

Forward references
Forward references are not allowed, to be compliant with SPV_KHR_non_semantic_info.

However, later while defining the DebugTypeComposite instruction, it says:

Members must be the <id>s of DebugTypeMember, DebugFunction, or DebugTypeInheritance. This could be a forward reference.

This statement contradicts the initial spec-wide statement, and caused some issues:

It seems that for those instruction to correctly support HLSL objects, we need to allow circular dependencies, hence forward references.

The spec mentions forward references should not be allowed to comply with SPV_KHR_non_semantic_info, but seems like this spec doesn't mention anything regarding forward references: http://htmlpreview.github.io/?https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/KHR/SPV_KHR_non_semantic_info.html

Since the only allowed values as members are other non-semantic instructions, it seems same to allow forward references, but maybe I'm missing something.

alan-baker commented 1 year ago

The SPIR WG discussed this in the 2023-05-24 meeting. Before we decide on the best path forward we wanted clarification on DXC's implementation. Is it possible to generate the instructions without forward references? If it can't be done we'll have to figure out the best way to allow forward references. If it can be done how hard is it? If it is onerous on implementations to avoid forward references that would also sway things towards changing the specs. If it isn't too bad on the other hand, it might simply be better to disallow the forward reference.

s-perron commented 1 year ago

Either way, the spec should be changed. If we choose to disallow forward references, then the DebugTypeComposite instruction description needs to remove the statement that a member could be a forward reference.

alan-baker commented 1 year ago

Either way, the spec should be changed. If we choose to disallow forward references, then the DebugTypeComposite instruction description needs to remove the statement that a member could be a forward reference.

Agreed, but disallowing forward references is less invasive potentially. So that might be preferred unless tooling cannot absorb the requirement.

greg-lunarg commented 1 year ago

I think the way that Shader.DebugInfo is currently defined (no forward references), certain legal GLSL and HLSL constructs cannot be expressed in Shader.DebugInfo.

I think we should hear from @baldurk why he thought we could not have forward references with a NonSemantic extension in general, and Shader.DebugInfo specifically.

What if we agreed that all uses of ids by nonsemantic instructions can be forward references?

If we decide that we cannot have forward references in Shader.DebugInfo, a thought: if we could redo the way we define structs in SPIR-V and/or DebugInfo, I think one possible way to handle circular references in structs to avoid needing forward references is the following:

%s = OpNewTypeStruct OpNewStructMember %s, 0, %type0 OpNewStructMember %s, 1, %type1 ... OpNewStructMember %s, N, %typeN

Member functions and types would work similarly.

arcady-lunarg commented 1 year ago

There is a related issue in glslang as well, where it is not handling forward references when generating DebugInfo: https://github.com/KhronosGroup/glslang/issues/3189

baldurk commented 1 year ago

My memory is imperfect and I've failed to dig up a specific note of it so I think it was lost in an ephemeral conversation somewhere. Unless I'm mistaken the requirement for all non-semantic parameters to be IDs and non-forward references both came at the same time from the spirv-tools folks, because they wanted to be able to process and transform non-semantic instruction sets without having to understand what the set did. Originally I didn't have any restrictions since the extension was purely meant to be a way of including side-band information in SPIR-V for tools that did not affect the semantic meaning of the module.

I have no problem with allowing it as it wasn't a restriction I wanted in the first place, but I'm not sure we can safely change that now.

In terms of avoiding forward references I think something along the lines of greg's suggestion of breaking up the member list from the struct definition seems best. There's a few different ways I could see to formulate it but none of them are backwards compatible so they'll all require a new extinst set. The mention of allowing forward references in DebugTypeComposite is essentially a bug and leftover from the OpenCL version, and should be removed I think.

As to the parent non-semantic extension itself not calling out forward references being disallowed, I think that was intended to be implicit from SPIR-V default not allowing them on instructions, and only the restriction about the use of result IDs being detailed since that is not a requirement in base SPIR-V (and as above, both were motivated by allowing tools to make assumptions about the grammar of unknown non-semantic extinst sets). Maybe the extension spec should be updated to be explicit about that?

Keenuts commented 1 year ago

Thanks for all the answers 😊

if we could redo the way we define structs in SPIR-V and/or DebugInfo

Or remove the method definition from the member list, as we do in spirv? But I'd imagine that's a no-go as we loose the debug info that a function is linked to an object (We could guess since there is a "this" parameter, but it's not explicit anymore)

they wanted to be able to process and transform non-semantic instruction sets without having to understand what the set did

Is that true today? Seems like the DebugInfo instruction set requires those non-semantic to be located at specific places in the code (like DebugScope that should be in a function body). So when running the inlining pass, the scope instruction must correctly be handled by ex, and we remove the ones we don't need.

What seems to be true is what the spec says about non-semantic: an instruction that has no semantic impact, and can be safely removed from the module.

In our case, we could either remove all non-semantic instruction and process the code, or correctly handle them, with or without forward references no?

baldurk commented 1 year ago

Honestly it never really made any sense to me. I don't know how a tool can do anything other than completely strip an unknown non-semantic extinst set if it was making modifications, because it doesn't know what the meaning is or if it will be broken by rearranging or changing other things in the module. Regardless of whether or not it's grammatically correct still, you don't know what those instructions represent. That's what I remember being the reason for the restriction though - e.g. this is the reason DebugFunctionDefinition was added rather than referencing the OpFunction ahead of time.

alan-baker commented 1 year ago

Structs in SPIR-V do not allow forward references. It is assumed that any cycle in the types can be solved using OpTypeForwardPointer. Should the debug instructions have a similar mechanism? Otherwise, I'm not seeing why forward references would be necessary.

Keenuts commented 1 year ago

Structs in SPIR-V do not allow forward references. It is assumed that any cycle in the types can be solved using OpTypeForwardPointer.

Yes, but the issue here is around member functions, which are not defined in the struct in SPIR-V. It seems in SPIR-V there is only a this parameter for functions, and then forward reference on the call site.

Only debug instruction defines them in the struct instruction so there is a link between the struct definition and the function definition. So we can probably add a new instruction type to have some kind of prototype and ignore forward references.

But once again, this new instruction requirement only makes sense IMO if there is a strong need to disallow forward references. So far I don't quite see the real reason behind this restriction:

Am I missing something?

greg-lunarg commented 1 year ago

Structs in SPIR-V do not allow forward references. It is assumed that any cycle in the types can be solved using OpTypeForwardPointer.

@alan-baker OpTypeForwardPointer does not get rid of the need for a forward reference. It CONTAINS a forward reference. So it and instructions like it are not a way to avoid forward references in Shader.DebugInfo.

Given the current way to define structs in Shader.DebugInfo, there is no way for a struct to contain a pointer to its own type without a forward reference. So we either have to allow forward references or change the way we define structs. There seems to be no other option.

My vote is to allow forward references. Unless someone comes up with a reason that this won't work, I suggest we move ahead with this.

alan-baker commented 1 year ago

Structs in SPIR-V do not allow forward references. It is assumed that any cycle in the types can be solved using OpTypeForwardPointer.

@alan-baker OpTypeForwardPointer does not get rid of the need for a forward reference. It CONTAINS a forward reference. So it and instructions like it are not a way to avoid forward references in Shader.DebugInfo.

Given the current way to define structs in Shader.DebugInfo, there is no way for a struct to contain a pointer to its own type without a forward reference. So we either have to allow forward references or change the way we define structs. There seems to be no other option.

My vote is to allow forward references. Unless someone comes up with a reason that this won't work, I suggest we move ahead with this.

One option is to add something akin to OpTypeForwardPointer to the debug instructions. I'm not saying that it's the best choice, but any choice will require modifications to the spec and that one seems somewhat in line with core SPIR-V.

@Keenuts, how is the member function represented? Is it a straight function declaration in the struct or a pointer to a function? This case seems more compelling than just data types for forward references, but I want to understand more. Could you attach a simple example to this bug (so we aren't chasing links) of how you'd like to represent that situation?

Keenuts commented 1 year ago

Considering this HLSL:

struct SomeObject {
    void some_method() { }
};

[numthreads(1, 1, 1)]
void compute(uint3 ids : SV_DispatchThreadID) {
    SomeObject obj;
    obj.some_method();
}

Here is the SPIR-V without debug info (-Od -T cs_6_0 -spirv)

               OpCapability Shader
               OpMemoryModel Logical GLSL450
               OpEntryPoint GLCompute %compute "compute" %gl_GlobalInvocationID
               OpExecutionMode %compute LocalSize 1 1 1
               OpSource HLSL 600
               OpName %compute "compute"
               OpName %param_var_ids "param.var.ids"
               OpName %src_compute "src.compute"
               OpName %ids "ids"
               OpName %bb_entry "bb.entry"
               OpName %SomeObject "SomeObject"
               OpName %obj "obj"
               OpName %SomeObject_some_method "SomeObject.some_method"
               OpName %param_this "param.this"
               OpName %bb_entry_0 "bb.entry"
               OpDecorate %gl_GlobalInvocationID BuiltIn GlobalInvocationId
       %uint = OpTypeInt 32 0
     %v3uint = OpTypeVector %uint 3
%_ptr_Input_v3uint = OpTypePointer Input %v3uint
       %void = OpTypeVoid
          %7 = OpTypeFunction %void
%_ptr_Function_v3uint = OpTypePointer Function %v3uint
         %14 = OpTypeFunction %void %_ptr_Function_v3uint
 %SomeObject = OpTypeStruct
%_ptr_Function_SomeObject = OpTypePointer Function %SomeObject
         %22 = OpTypeFunction %void %_ptr_Function_SomeObject
%gl_GlobalInvocationID = OpVariable %_ptr_Input_v3uint Input
    %compute = OpFunction %void None %7
          %8 = OpLabel
%param_var_ids = OpVariable %_ptr_Function_v3uint Function
         %11 = OpLoad %v3uint %gl_GlobalInvocationID
         %12 = OpFunctionCall %void %src_compute %param_var_ids
               OpReturn
               OpFunctionEnd
%src_compute = OpFunction %void None %14
        %ids = OpFunctionParameter %_ptr_Function_v3uint
   %bb_entry = OpLabel
        %obj = OpVariable %_ptr_Function_SomeObject Function
         %20 = OpFunctionCall %void %SomeObject_some_method %obj
               OpReturn
               OpFunctionEnd
%SomeObject_some_method = OpFunction %void None %22
 %param_this = OpFunctionParameter %_ptr_Function_SomeObject
 %bb_entry_0 = OpLabel
               OpReturn
               OpFunctionEnd

and with debug info: (-Od -T cs_6_0 -spirv -fspv-debug=vulkan -Vd, -Vd to workaround the bug)

               OpCapability Shader
               OpExtension "SPV_KHR_non_semantic_info"
          %1 = OpExtInstImport "NonSemantic.Shader.DebugInfo.100"
               OpMemoryModel Logical GLSL450
               OpEntryPoint GLCompute %compute "compute" %gl_GlobalInvocationID
               OpExecutionMode %compute LocalSize 1 1 1
          %4 = OpString "/tmp/repro.hlsl"
         %15 = OpString "SomeObject"
         %22 = OpString "SomeObject.some_method"
         %23 = OpString ""
         %27 = OpString "this"
         %30 = OpString "uint"
         %35 = OpString "compute"
         %39 = OpString "obj"
         %44 = OpString "ids"
         %47 = OpString "1710f68d"
         %48 = OpString " -E compute -T cs_6_0 -spirv -Od -fspv-debug=vulkan -Vd -Qembed_debug"
               OpName %compute "compute"
               OpName %param_var_ids "param.var.ids"
               OpName %src_compute "src.compute"
               OpName %ids "ids"
               OpName %bb_entry "bb.entry"
               OpName %SomeObject "SomeObject"
               OpName %obj "obj"
               OpName %SomeObject_some_method "SomeObject.some_method"
               OpName %param_this "param.this"
               OpName %bb_entry_0 "bb.entry"
               OpDecorate %gl_GlobalInvocationID BuiltIn GlobalInvocationId
       %uint = OpTypeInt 32 0
    %uint_32 = OpConstant %uint 32
     %v3uint = OpTypeVector %uint 3
%_ptr_Input_v3uint = OpTypePointer Input %v3uint
       %void = OpTypeVoid
     %uint_1 = OpConstant %uint 1
     %uint_4 = OpConstant %uint 4
     %uint_5 = OpConstant %uint 5
     %uint_0 = OpConstant %uint 0
     %uint_8 = OpConstant %uint 8
     %uint_3 = OpConstant %uint 3
     %uint_2 = OpConstant %uint 2
    %uint_24 = OpConstant %uint 24
   %uint_288 = OpConstant %uint 288
     %uint_6 = OpConstant %uint 6
    %uint_47 = OpConstant %uint 47
     %uint_7 = OpConstant %uint 7
    %uint_16 = OpConstant %uint 16
    %uint_20 = OpConstant %uint 20
         %50 = OpTypeFunction %void
%_ptr_Function_v3uint = OpTypePointer Function %v3uint
     %uint_9 = OpConstant %uint 9
         %60 = OpTypeFunction %void %_ptr_Function_v3uint
 %SomeObject = OpTypeStruct
%_ptr_Function_SomeObject = OpTypePointer Function %SomeObject
    %uint_14 = OpConstant %uint 14
    %uint_21 = OpConstant %uint 21
         %79 = OpTypeFunction %void %_ptr_Function_SomeObject
    %uint_26 = OpConstant %uint 26
%gl_GlobalInvocationID = OpVariable %_ptr_Input_v3uint Input
         %10 = OpExtInst %void %1 DebugSource %4
         %11 = OpExtInst %void %1 DebugCompilationUnit %uint_1 %uint_4 %10 %uint_5
         %17 = OpExtInst %void %1 DebugTypeComposite %15 %uint_1 %10 %uint_1 %uint_8 %11 %15 %uint_0 %uint_3 %20
         %21 = OpExtInst %void %1 DebugTypeFunction %uint_3 %void %17
         %20 = OpExtInst %void %1 DebugFunction %22 %21 %10 %uint_2 %uint_5 %17 %23 %uint_3 %uint_2
         %25 = OpExtInst %void %1 DebugLexicalBlock %10 %uint_2 %uint_24 %20
         %28 = OpExtInst %void %1 DebugLocalVariable %27 %17 %10 %uint_2 %uint_5 %20 %uint_288 %uint_1
         %31 = OpExtInst %void %1 DebugTypeBasic %30 %uint_32 %uint_6 %uint_0
         %33 = OpExtInst %void %1 DebugTypeVector %31 %uint_3
         %34 = OpExtInst %void %1 DebugTypeFunction %uint_3 %void %33
         %36 = OpExtInst %void %1 DebugFunction %35 %34 %10 %uint_6 %uint_1 %11 %23 %uint_3 %uint_6
         %37 = OpExtInst %void %1 DebugLexicalBlock %10 %uint_6 %uint_47 %36
         %40 = OpExtInst %void %1 DebugLocalVariable %39 %17 %10 %uint_7 %uint_16 %37 %uint_4
         %43 = OpExtInst %void %1 DebugExpression
         %45 = OpExtInst %void %1 DebugLocalVariable %44 %33 %10 %uint_6 %uint_20 %36 %uint_4 %uint_1
         %49 = OpExtInst %void %1 DebugEntryPoint %36 %11 %47 %48
    %compute = OpFunction %void None %50
         %51 = OpLabel
%param_var_ids = OpVariable %_ptr_Function_v3uint Function
         %54 = OpExtInst %void %1 DebugFunctionDefinition %36 %compute
         %55 = OpLoad %v3uint %gl_GlobalInvocationID
         %56 = OpFunctionCall %void %src_compute %param_var_ids
         %58 = OpExtInst %void %1 DebugLine %10 %uint_9 %uint_9 %uint_1 %uint_1
               OpReturn
               OpFunctionEnd
%src_compute = OpFunction %void None %60
        %ids = OpFunctionParameter %_ptr_Function_v3uint
   %bb_entry = OpLabel
        %obj = OpVariable %_ptr_Function_SomeObject Function
         %66 = OpExtInst %void %1 DebugScope %36
         %67 = OpExtInst %void %1 DebugLine %10 %uint_6 %uint_6 %uint_14 %uint_20
         %69 = OpExtInst %void %1 DebugDeclare %45 %ids %43
         %70 = OpExtInst %void %1 DebugScope %37
         %71 = OpExtInst %void %1 DebugLine %10 %uint_7 %uint_7 %uint_5 %uint_16
         %72 = OpExtInst %void %1 DebugDeclare %40 %obj %43
         %73 = OpExtInst %void %1 DebugLine %10 %uint_8 %uint_8 %uint_5 %uint_21
         %75 = OpFunctionCall %void %SomeObject_some_method %obj
         %77 = OpExtInst %void %1 DebugScope %36
         %78 = OpExtInst %void %1 DebugLine %10 %uint_9 %uint_9 %uint_1 %uint_1
               OpReturn
               OpFunctionEnd
%SomeObject_some_method = OpFunction %void None %79
 %param_this = OpFunctionParameter %_ptr_Function_SomeObject
 %bb_entry_0 = OpLabel
         %82 = OpExtInst %void %1 DebugScope %20
         %83 = OpExtInst %void %1 DebugLine %10 %uint_2 %uint_2 %uint_5 %uint_26
         %85 = OpExtInst %void %1 DebugDeclare %28 %param_this %43
         %86 = OpExtInst %void %1 DebugNoLine
         %87 = OpExtInst %void %1 DebugFunctionDefinition %20 %SomeObject_some_method
         %88 = OpExtInst %void %1 DebugScope %25
         %89 = OpExtInst %void %1 DebugScope %20
         %90 = OpExtInst %void %1 DebugLine %10 %uint_2 %uint_2 %uint_26 %uint_26
               OpReturn
               OpFunctionEnd

As you can see, the SPIR-V way to handle methods is to just give a this parameter. The OpTypeStruct for SomeObject is empty, no reference to the method. The OpTypeFunction defines a regular function, with a single parameter of type SomeObject. There is no way to differentiate between a method, on a normal method taking SomeObject as parameter. Up to the OpFunctionCall, no forward reference. There is no need as there is no link between the object and the method, meaning no self-reference.

Now, for the debug info, the issue is with those 3 lines:

%17 = OpExtInst %void %1 DebugTypeComposite %15 %uint_1 %10 %uint_1 %uint_8 %11 %15 %uint_0 %uint_3 %20
%21 = OpExtInst %void %1 DebugTypeFunction %uint_3 %void %17
%20 = OpExtInst %void %1 DebugFunction %22 %21 %10 %uint_2 %uint_5 %17 %23 %uint_3 %uint_2

The DebugTypeComposite for SomeObject references the debug instruction for the method (not like SPIR-V composites). This is required so the debugger can show "some_method is a method of SomeObject". Then, the DebugFunction for some_method references the DebugTypeFunction which lists the parameters. Because there is a this parameter, and we should link it to a debug type, the loop is closed. Also, the DebugFunction directly references the composite because it is its scope.

Right now, those instruction are defined as follows:

DebugFunction <name> <type> [...] <scope>
DebugTypeFunction <flags> <returnType> <param0> <param1> ...
DebugTypeComposite <name> [...] <member1> <member2>

Since we know member function will take a "this" as param, we could remove it, and have it being implicit. Remains the scope, which could also be implicit.

%1 = DebugTypeMemberFunction <flags> <returnType> <param1> ...
%2 = DebugMemberFunction <name> %1 [...]
%3 = DebugTypeComposite <name> [...] <member1> %2

This would allow us to remove forward references. but I would be against those implicit meanings just for the sake of not having forward references. As I mentionned in https://github.com/KhronosGroup/SPIRV-Registry/issues/203#issuecomment-1562779308, I don't see why we should disallow forward references. So I'd really be in favor to not add implicit meaning, and just allow forward references. Which only requires a small fix in the spec to say "not allowed unless specified" to it's less ambiguous.

greg-lunarg commented 1 year ago

@alan-baker @Keenuts I would like to repeat my assertion that an OpTypeForwardPointer-like operator would NOT remove the need for forward references, even if the changes for member functions above were made.

OpTypeForwardPointer actually contains a forward reference, so an OpTypeForwardPointer-like operator would NOT remove the need for forward references for structs that contains pointers to the struct's type.

OpTypeForwardPointer only allows the programmer to proactively specify the pointee's class.

greg-lunarg commented 1 year ago

The only "problem" I see that is created by allowing forward references in NonSemantic extended sets is that tools that wish to ignore but not remove Nonsemantic sets will need to assume that all of the sets ids can be forward references, which will require two passes of the SPIR-V to assure that needed instructions are not removed. This is just an efficiency problem, not an insurmountable obstacle to accepting forward references.

Unfortunately, allowing forward references in NonSemantic sets could break some existing tools which assume no forward references, so this change will require bumping the major revisions of all NonSemantic sets that take this change, and updating the tools making this assumption. Note that this is just an inconvenience, not an insurmountable obstacle to accepting forwared references.

So I see no insurmountable obstacles to accepting forward references in NonSemantic sets.

alan-baker commented 1 year ago

@alan-baker @Keenuts I would like to repeat my assertion that an OpTypeForwardPointer-like operator would NOT remove the need for forward references, even if the changes for member functions above were made.

OpTypeForwardPointer actually contains a forward reference, so an OpTypeForwardPointer-like operator would NOT remove the need for forward references for structs that contains pointers to the struct's type.

OpTypeForwardPointer only allows the programmer to proactively specify the pointee's class.

I'm not saying the forward reference disappears, but the core SPIR-V spec handles cases forward references are allowed for this type of situation that way. So one such option would an analogous sort construct and rule in the debug extended instruction set.

The example @Keenuts provided is interesting. The parameter for the member function is a structure itself and not a pointer to the structure (maybe because you'd need a function for each storage class?). So adding a forward declaration type construct might be more invasive than anticipated.

SPIR-V generally tries to limit the availability of forward references. Changing non-semantic info to allow forward references everywhere feels like it goes too far, but only saying if explicitly allowed in the instruction is no help either.

Keenuts commented 1 year ago

Unfortunately, allowing forward references in NonSemantic sets could break some existing tools which assume no forward references

That's a valid point. Eventhough the non-semantic spec doesn't forbid forward references, the SPIRV spec itself does forbid them, except in some listed cases. Meaning it would be valid for tools to not expect forward references on extended instructions.

@alan-baker : If we add an OpTypeForwardPointer-like instruction in the debug set, wouldn't the SPIR-V spec also require an update to specify OpExtInst could contains forward references?

The parameter for the member function is a structure itself and not a pointer to the structure (maybe because you'd need a function for each storage class?).

Seems like so: SPIR-V does declare 2 composite for each storage type. But the debug instruction are only generated once.

Here is another HLSL example to showcase the 2 declarations:

struct SomeObject {
    int value;
    int some_method() { return value; }
};

RWStructuredBuffer<SomeObject> input;

[numthreads(1, 1, 1)]
void compute(uint3 ids : SV_DispatchThreadID) {
    SomeObject obj;

    int a = obj.some_method();
    int b = input[ids.x].some_method();
}

Compiled with validation, legalization and optimizations disabled, and debug enabled

               OpCapability Shader
               OpExtension "SPV_KHR_non_semantic_info"
          %1 = OpExtInstImport "NonSemantic.Shader.DebugInfo.100"
               OpMemoryModel Logical GLSL450
               OpEntryPoint GLCompute %compute "compute" %gl_GlobalInvocationID
               OpExecutionMode %compute LocalSize 1 1 1
          %4 = OpString "/tmp/repro.hlsl"
          %5 = OpString "int"
          %6 = OpString "value"
          %7 = OpString "SomeObject"
          %8 = OpString "SomeObject.some_method"
          %9 = OpString ""
         %10 = OpString "this"
         %11 = OpString "uint"
         %12 = OpString "compute"
         %13 = OpString "b"
         %14 = OpString "a"
         %15 = OpString "obj"
         %16 = OpString "ids"
         %17 = OpString "8f3f77ed"
         %18 = OpString " -E compute -T cs_6_6 -Vd -Od -spirv -fspv-debug=vulkan -fspv-print-all -Qembed_debug"
         %19 = OpString "@type.RWStructuredBuffer.SomeObject"
         %20 = OpString "type.RWStructuredBuffer.SomeObject"
         %21 = OpString "TemplateParam"
         %22 = OpString "input"
               OpName %type_RWStructuredBuffer_SomeObject "type.RWStructuredBuffer.SomeObject"
               OpName %SomeObject "SomeObject"
               OpMemberName %SomeObject 0 "value"
               OpName %input "input"
               OpName %compute "compute"
               OpName %param_var_ids "param.var.ids"
               OpName %src_compute "src.compute"
               OpName %ids "ids"
               OpName %bb_entry "bb.entry"
               OpName %SomeObject_0 "SomeObject"
               OpMemberName %SomeObject_0 0 "value"
               OpName %obj "obj"
               OpName %a "a"
               OpName %b "b"
               OpName %SomeObject_some_method "SomeObject.some_method"
               OpName %param_this "param.this"
               OpName %bb_entry_0 "bb.entry"
               OpDecorate %gl_GlobalInvocationID BuiltIn GlobalInvocationId
               OpDecorate %input DescriptorSet 0
               OpDecorate %input Binding 0
               OpMemberDecorate %SomeObject 0 Offset 0
               OpDecorate %_runtimearr_SomeObject ArrayStride 4
               OpMemberDecorate %type_RWStructuredBuffer_SomeObject 0 Offset 0
               OpDecorate %type_RWStructuredBuffer_SomeObject BufferBlock
        %int = OpTypeInt 32 1
      %int_0 = OpConstant %int 0
       %uint = OpTypeInt 32 0
    %uint_32 = OpConstant %uint 32
 %SomeObject = OpTypeStruct %int
%_runtimearr_SomeObject = OpTypeRuntimeArray %SomeObject
%type_RWStructuredBuffer_SomeObject = OpTypeStruct %_runtimearr_SomeObject
%_ptr_Uniform_type_RWStructuredBuffer_SomeObject = OpTypePointer Uniform %type_RWStructuredBuffer_SomeObject
     %v3uint = OpTypeVector %uint 3
%_ptr_Input_v3uint = OpTypePointer Input %v3uint
       %void = OpTypeVoid
     %uint_4 = OpConstant %uint 4
     %uint_0 = OpConstant %uint 0
     %uint_1 = OpConstant %uint 1
     %uint_5 = OpConstant %uint 5
     %uint_2 = OpConstant %uint 2
     %uint_9 = OpConstant %uint 9
     %uint_3 = OpConstant %uint 3
     %uint_8 = OpConstant %uint 8
    %uint_23 = OpConstant %uint 23
   %uint_288 = OpConstant %uint 288
     %uint_6 = OpConstant %uint 6
    %uint_47 = OpConstant %uint 47
    %uint_13 = OpConstant %uint 13
    %uint_12 = OpConstant %uint 12
    %uint_10 = OpConstant %uint 10
    %uint_16 = OpConstant %uint 16
    %uint_20 = OpConstant %uint 20
         %63 = OpTypeFunction %void
%_ptr_Function_v3uint = OpTypePointer Function %v3uint
    %uint_14 = OpConstant %uint 14
         %66 = OpTypeFunction %void %_ptr_Function_v3uint
%SomeObject_0 = OpTypeStruct %int
%_ptr_Function_SomeObject_0 = OpTypePointer Function %SomeObject_0
%_ptr_Function_int = OpTypePointer Function %int
    %uint_29 = OpConstant %uint 29
%_ptr_Function_uint = OpTypePointer Function %uint
    %uint_19 = OpConstant %uint 19
%_ptr_Uniform_SomeObject = OpTypePointer Uniform %SomeObject
    %uint_24 = OpConstant %uint 24
    %uint_38 = OpConstant %uint 38
         %75 = OpTypeFunction %int %_ptr_Function_SomeObject_0
    %uint_39 = OpConstant %uint 39
    %uint_25 = OpConstant %uint 25
      %input = OpVariable %_ptr_Uniform_type_RWStructuredBuffer_SomeObject Uniform
%gl_GlobalInvocationID = OpVariable %_ptr_Input_v3uint Input
         %78 = OpExtInst %void %1 DebugTypeBasic %5 %uint_32 %uint_4 %uint_0
         %79 = OpExtInst %void %1 DebugSource %4
         %80 = OpExtInst %void %1 DebugCompilationUnit %uint_1 %uint_4 %79 %uint_5
         %81 = OpExtInst %void %1 DebugTypeMember %6 %78 %79 %uint_2 %uint_9 %uint_0 %uint_32 %uint_3
         %82 = OpExtInst %void %1 DebugTypeComposite %7 %uint_1 %79 %uint_1 %uint_8 %80 %7 %uint_32 %uint_3 %81 %83
         %84 = OpExtInst %void %1 DebugTypeFunction %uint_3 %78 %82
         %83 = OpExtInst %void %1 DebugFunction %8 %84 %79 %uint_3 %uint_5 %82 %9 %uint_3 %uint_3
         %85 = OpExtInst %void %1 DebugLexicalBlock %79 %uint_3 %uint_23 %83
         %86 = OpExtInst %void %1 DebugLocalVariable %10 %82 %79 %uint_3 %uint_5 %83 %uint_288 %uint_1
         %87 = OpExtInst %void %1 DebugTypeBasic %11 %uint_32 %uint_6 %uint_0
         %88 = OpExtInst %void %1 DebugTypeVector %87 %uint_3
         %89 = OpExtInst %void %1 DebugTypeFunction %uint_3 %void %88
         %90 = OpExtInst %void %1 DebugFunction %12 %89 %79 %uint_9 %uint_1 %80 %9 %uint_3 %uint_9
         %91 = OpExtInst %void %1 DebugLexicalBlock %79 %uint_9 %uint_47 %90
         %92 = OpExtInst %void %1 DebugLocalVariable %13 %78 %79 %uint_13 %uint_9 %91 %uint_4
         %93 = OpExtInst %void %1 DebugLocalVariable %14 %78 %79 %uint_12 %uint_9 %91 %uint_4
         %94 = OpExtInst %void %1 DebugLocalVariable %15 %82 %79 %uint_10 %uint_16 %91 %uint_4
         %95 = OpExtInst %void %1 DebugExpression
         %96 = OpExtInst %void %1 DebugLocalVariable %16 %88 %79 %uint_9 %uint_20 %90 %uint_4 %uint_1
         %97 = OpExtInst %void %1 DebugInfoNone
         %98 = OpExtInst %void %1 DebugTypeComposite %19 %uint_0 %79 %uint_0 %uint_0 %80 %20 %97 %uint_3
         %99 = OpExtInst %void %1 DebugTypeMember %6 %78 %79 %uint_2 %uint_9 %uint_0 %uint_32 %uint_3
        %100 = OpExtInst %void %1 DebugTypeComposite %7 %uint_1 %79 %uint_1 %uint_8 %80 %7 %uint_32 %uint_3 %99 %83
        %101 = OpExtInst %void %1 DebugTypeTemplateParameter %21 %100 %97 %79 %uint_0 %uint_0
        %102 = OpExtInst %void %1 DebugTypeTemplate %98 %101
        %103 = OpExtInst %void %1 DebugGlobalVariable %22 %102 %79 %uint_6 %uint_32 %80 %22 %input %uint_8
        %104 = OpExtInst %void %1 DebugEntryPoint %90 %80 %17 %18
    %compute = OpFunction %void None %63
        %105 = OpLabel
%param_var_ids = OpVariable %_ptr_Function_v3uint Function
        %106 = OpExtInst %void %1 DebugFunctionDefinition %90 %compute
        %107 = OpLoad %v3uint %gl_GlobalInvocationID
               OpStore %param_var_ids %107
        %108 = OpFunctionCall %void %src_compute %param_var_ids
        %109 = OpExtInst %void %1 DebugLine %79 %uint_14 %uint_14 %uint_1 %uint_1
               OpReturn
               OpFunctionEnd
%src_compute = OpFunction %void None %66
        %ids = OpFunctionParameter %_ptr_Function_v3uint
   %bb_entry = OpLabel
        %obj = OpVariable %_ptr_Function_SomeObject_0 Function
          %a = OpVariable %_ptr_Function_int Function
          %b = OpVariable %_ptr_Function_int Function
        %110 = OpExtInst %void %1 DebugScope %90
        %111 = OpExtInst %void %1 DebugLine %79 %uint_9 %uint_9 %uint_14 %uint_20
        %112 = OpExtInst %void %1 DebugDeclare %96 %ids %95
        %113 = OpExtInst %void %1 DebugScope %91
        %114 = OpExtInst %void %1 DebugLine %79 %uint_10 %uint_10 %uint_5 %uint_16
        %115 = OpExtInst %void %1 DebugDeclare %94 %obj %95
        %116 = OpExtInst %void %1 DebugLine %79 %uint_12 %uint_12 %uint_13 %uint_29
        %117 = OpFunctionCall %int %SomeObject_some_method %obj
        %118 = OpExtInst %void %1 DebugLine %79 %uint_12 %uint_12 %uint_5 %uint_29
               OpStore %a %117
        %119 = OpExtInst %void %1 DebugDeclare %93 %a %95
        %120 = OpExtInst %void %1 DebugLine %79 %uint_13 %uint_13 %uint_19 %uint_23
        %121 = OpAccessChain %_ptr_Function_uint %ids %int_0
        %122 = OpLoad %uint %121
        %123 = OpExtInst %void %1 DebugLine %79 %uint_13 %uint_13 %uint_13 %uint_24
        %124 = OpAccessChain %_ptr_Uniform_SomeObject %input %int_0 %122
        %125 = OpExtInst %void %1 DebugLine %79 %uint_13 %uint_13 %uint_13 %uint_38
        %126 = OpFunctionCall %int %SomeObject_some_method %124
        %127 = OpExtInst %void %1 DebugLine %79 %uint_13 %uint_13 %uint_5 %uint_38
               OpStore %b %126
        %128 = OpExtInst %void %1 DebugDeclare %92 %b %95
        %129 = OpExtInst %void %1 DebugScope %90
        %130 = OpExtInst %void %1 DebugLine %79 %uint_14 %uint_14 %uint_1 %uint_1
               OpReturn
        %131 = OpExtInst %void %1 DebugNoScope
               OpFunctionEnd
%SomeObject_some_method = OpFunction %int None %75
 %param_this = OpFunctionParameter %_ptr_Function_SomeObject_0
 %bb_entry_0 = OpLabel
        %132 = OpExtInst %void %1 DebugScope %83
        %133 = OpExtInst %void %1 DebugLine %79 %uint_3 %uint_3 %uint_5 %uint_39
        %134 = OpExtInst %void %1 DebugDeclare %86 %param_this %95
        %135 = OpExtInst %void %1 DebugNoLine
        %136 = OpExtInst %void %1 DebugFunctionDefinition %83 %SomeObject_some_method
        %137 = OpExtInst %void %1 DebugScope %85
        %138 = OpExtInst %void %1 DebugLine %79 %uint_3 %uint_3 %uint_32 %uint_32
        %139 = OpAccessChain %_ptr_Function_int %param_this %int_0
        %140 = OpLoad %int %139
        %141 = OpExtInst %void %1 DebugLine %79 %uint_3 %uint_3 %uint_25 %uint_32
               OpReturnValue %140
        %142 = OpExtInst %void %1 DebugNoScope
               OpFunctionEnd

@alan-baker, @greg-lunarg: OpTypeForwardPointer doesn't remove the forward reference, but simplifies the handling as it's one of the few locations we are allowed to do one. I could see 2 ways of implementing what we want: at the non-semantic debug level, or the SPIR-V OpExtInst level.

If we want to do it at the non-semantic debug level, it would require adding the following:

DebugForwardReference

Define a lists of forward-referenced IDs.
Result Type must be OpTypeVoid

DebugForwardReference <id1> <id2> <id3>

Usages would be as follows:

       OpExtInst %void %1 DebugForwardReferences %83
 %82 = OpExtInst %void %1 DebugTypeComposite [...] %83
 %84 = OpExtInst %void %1 DebugTypeFunction [...] %82
 %83 = OpExtInst %void %1 DebugFunction [...] %84 [...] %82 [...]

What bugs me here is we need to allow forward references in a quite large scope (OpExtInst + non-semantic instructions).

The other option would be to add an OpExtForwardReference instruction at the SPIR-V level:

It would require a new spirv version, and I have no idea how it's done, but this seems to be the less intrusive option. WDYT?

alan-baker commented 1 year ago

@alan-baker : If we add an OpTypeForwardPointer-like instruction in the debug set, wouldn't the SPIR-V spec also require an update to specify OpExtInst could contains forward references?

My thought would be more like we'd have to change the debug instruction set to avoid it. Something like:

%1 = OpExtInst %void %1 DebugTypeCompositeDeclare [...] ; but no members
%2 = OpExtInst %void %1 DebugTypeCompositeMember %1 [...] ; member 1 of %1
...
%n = OpExtInst %void %1 DebugTypeCompositeMember %1 [...] ; member n of %1

Then we the struct could be declared as a scope and the members could be filled in later. Obviously this would require a version bump and complicate tooling.

The other option would be to add an OpExtForwardReference instruction at the SPIR-V level:

This is an interesting idea. It could be it's own extension to modify the core (and SPV_KHR_non_semantic_info). At least tooling would be prepared for such cases if they supported the extension. I'd feel better about limiting the scope too, but I'd want to consider being even more restrictive to start and say only non-semantic instruction sets could use the new instruction as a start.

Keenuts commented 1 year ago
%1 = OpExtInst %void %1 DebugTypeCompositeDeclare [...] ; but no members
%2 = OpExtInst %void %1 DebugTypeCompositeMember %1 [...] ; member 1 of %1
...
%n = OpExtInst %void %1 DebugTypeCompositeMember %1 [...] ; member n of %1

Then we the struct could be declared as a scope and the members could be filled in later. Obviously this would require a version bump and complicate tooling.

So this would invert the DebugTypeMember usage we have today, and require an additional type definition, but could work yes:

  1 struct SomeObject {
  2     int value;
  3     int some_method() { return value; }
  4 };

would be represented as

1 = composite "SomeObject"
2 = typeBasic "int"
3 = typeFunction return=2 param0=1
4 = function type=3 scope=1
5 = member "value" parent=1 type=2
6 = member "some_method" parent=1 type=4

Doesn't seem to complicate tooling much, and it does remove any forward reference for IDs (we just have an incomplete type for a while). I'd be fine with that.

greg-lunarg commented 1 year ago

I am still preferring allowing forward refs in NonSemantic extended instruction sets (I think it would be less disruptive and less confusing than changing struct definitions in Shader.DebugInfo), and I am liking the idea above to add this through a new instruction or instruction form.

One idea is to add a decoration to a NonSemantic OpExtInstImport instruction to indicate it contains forward references.

Another idea is to allow the string "FwdRef" to be added after "NonSemantic" to indicate the instructions may contain forward references. This way tools do not have to assume all NonSemantic sets contain forward references.

I am concerned about having struct definitions in Shader.DebugInfo being different from those in OpenCL.DebugInfo or core SPIR-V. Such a difference I fear would be confusing to users and possibly add to validation logic burden.

greg-lunarg commented 1 year ago

FYI, I propose that while forward references would be allowed, we would still only allow limited and targeted forward references in Shader.DebugInfo, so tools that choose to understand this instruction set can limit the forward references it needs to watch for.

Keenuts commented 1 year ago

Hello!

So far, we have a few proposed ideas:

A. Change NonSemantic spec to allow forward reference.

Rule is simple, but too generic. Restricting the presence of forward references seem to be the default for SPIR-V. If we add an exception, we should limit its impact.

B. NonSemantic.Shader.DebugInfo.200 set, with DebugTypeCompositeDeclare and change in the member declaration.

This one seems simpler: we can keep the no-forward reference restriction. @greg-lunarg is concerned this would confuse users, and that it would make DebugInfo diverge a bit from the OpenCL equivalent. I'm not convinced there is a risk for user confusion, as who is actually reading SPIR-V debug instructions except debugger developers? If the spec is clear, how would that be confusing?

C. Change NonSemantic.Shader.DebugInfo.100 to allow forward references in the Member case.

This would makes it closer to OpenCL.DebugInfo.100, as this spec allows forward references for this specific case. Would go into Gregs' direction to reduce divergence. However, OpenCL.DebugInfo.100 is not a non-semantic instruction set. Main opponent is @alan-baker: NonSemantic should not have forward reference, unless we really cannot.

D. NonSemantic.Shader.DebugInfo.200 set, with DebugForwardReference type of instruction.

Needs allowing forward references in the spec anyway, but we have a specific instruction to limit the scope to (instead of a few like OpenCL.DebugInfo.100). I'm personally not convinced of the utility of this approach as we would need to both add an instruction, and allow forward references in non-semantic instructions. (Yes, limited to 1 instruction, but still).

E. Add a FwdRef suffix to NonSemantic instruction sets's name to allow forward references.

Clearly defined which NonSemantic can have forward reference or not. What I don't like is:

F. New SPIR-V instruction like OpExtForwardReference to allow some IDs to be forward declared in related extended instructions.

Adding this requires a spirv bump (or an extension like Alan suggested). What I like here is we don't only solve the problem for the DebugInfo.100 spec, but for all extended instruction sets, as each instruction set could now use this to declare forward references. (Maybe it could be a decorator instead?)

Main worry is: do we want to have an easy way to allow forward references?

I'd personally be in favor of the solution F, a new OpExtForwardReference instruction or decoration. Doesn't seem to intrusive, and could be use to remove extension-specific exceptions? (Maybe OpenCL.DebugInfo.200 could remove all exceptions, and rely on this?)

greg-lunarg commented 1 year ago

By Occam's Razor, the simplest solution should be chosen. That would be the solution that takes the least work to document and understand and causes the least work to the existing base and future development. I think that solution is allowing limited, targeted forward references.

Changes to struct type will require significant changes to dxc, glslang, renderdoc, and nearly all spirv-tools including spirv-val and spirv-opt.

Adding targeted forward references would generally require no changes to these tools since they already support OpenCL.DebugInfo which allows forward references. There may exist some user SPIR-V tools in the Vulkan universe that have to slightly change the way they do things to handle these. At this time, I am not aware of any.

Keenuts commented 1 year ago

Wouldn't option F, adding a new instruction goes into the "simplest solution" category?

MrSidims commented 1 year ago

We have recently added support of NonSemantic.Shader.DebugInfo.100 and NonSemantic.Shader.DebugInfo.200 extensions in https://github.com/KhronosGroup/SPIRV-LLVM-Translator . 'No forward references' requirement was not easy to achieve, nevertheless we managed to do that. In our case LLVM infrastructure helped here a lot, so we could postpone some translation to later and I can imagine that in not LLVM-based FE it can be harder.

Yet we had some issues with recursive debug info generation like the following: translation of DIDerivedType (member) that calls DICompositeType translation as its parent scope; translation of DICompositeType calls translation of its members (DIDerivedType with member tag) (just like the example in the issue header). We were able to achieve 'no forward references' even in this case (see test from https://github.com/KhronosGroup/SPIRV-LLVM-Translator/pull/2033 ):

; CHECK-SPIRV: ExtInst [[#]] [[#**Member1**:]] [[#]] DebugTypeMember [[#]] [[#]] [[#]] [[#]] [[#]] [[#]] [[#]] [[#]] {{$}}
; CHECK-SPIRV: ExtInst [[#]] [[#**Member2**:]] [[#]] DebugTypeMember [[#]] [[#]] [[#]] [[#]] [[#]] [[#]] [[#]] [[#]] {{$}} 
; CHECK-SPIRV: ExtInst [[#]] [[#]] [[#]] DebugTypeComposite [[#]] [[#]] [[#]] [[#]] [[#]] [[#]] [[#]] [[#]] [[#]] [[#**Member1**]] [[#**Member2**]] 

but I must acknowledge that the solution for this was a bit hacky and we left some 'dead' instructions on the way.

greg-lunarg commented 1 year ago

To be very clear, for the purposes of this issue, I am suggesting we allow forward references only where OpenCL.DebugInfo allows them. I am not suggesting that we allow every use to be a forward reference in Shader.DebugInfo.

greg-lunarg commented 1 year ago

Wouldn't option F, adding a new instruction goes into the "simplest solution" category?

I would certainly prefer OpExtForwardReference to DebugTypeCompositeDeclare. But it seems a little more permissive and general than it needs to be, and perhaps more permissive and general than it should be. I am worried it gives license to the user to have forward references anywhere they want.

My preference would be that forward references are allowed only where the related spec specifically allows them to be.

Also, please note, my proposal to add FwdRef to NonSemantic (or an option to OptExtInstImport) would not allow forward references anywhere in the extended instruction set. It would just be a warning to any indifferent tool that the instruction set contains forward references.

alan-baker commented 1 year ago

By Occam's Razor, the simplest solution should be chosen. That would be the solution that takes the least work to document and understand and causes the least work to the existing base and future development. I think that solution is allowing limited, targeted forward references.

I don't view having to modify the core SPIR-V spec and the non-semantic info extension as the simplest possible change. Allowing forward references means fixing those. The simplest change, in my opinion, is limited to the extended instruction set itself even if tooling needs to absorb some of the changes.

I don't require we make the simplest change, but my reference is not just allowing forward references in such a manner. I'd prefer B or F.

greg-lunarg commented 1 year ago

I don't view having to modify the core SPIR-V spec and the non-semantic info extension as the simplest possible change. Allowing forward references means fixing those.

@alan-baker Please note my suggestion does not involve changing SPIR-V core or the non-semantic info extension ie https://github.com/KhronosGroup/SPIRV-Registry/blob/main/extensions/KHR/SPV_KHR_non_semantic_info.html

My suggestion is the following:

1) Change NonSemantic.Shader.DebugInfo ie https://github.com/KhronosGroup/SPIRV-Registry/blob/main/nonsemantic/NonSemantic.Shader.DebugInfo.100.html such that wherever it says "Forward references are not allowed" add "except where explicitly allowed".

2) Bump the major version of NonSemantic.Shader.DebugInfo.

Notice no changes to SPIR-V core or NonSemantic extension are proposed or needed.

alan-baker commented 1 year ago

Notice no changes to SPIR-V core or NonSemantic extension are proposed or needed.

SPIR-V states the places forward references can appear:

Forward references (an operand that appears before the Result defining it) are allowed for:

So it is not enough for an extended instruction set to allow forward references as they are generally banned in core SPIR-V.

greg-lunarg commented 1 year ago

So it is not enough for an extended instruction set to allow forward references as they are generally banned in core SPIR-V.

I think this is an arguable point. I think a reasonable argument could be made that this language does not prohibit extensions or extended instruction sets from containing forward references. One would not expect an enumeration of extended instructions at this point, so one would not reasonably expect their absence to imply disapproval. And there is at least one example which seems to agree with this interpretation: OpenCL.DebugInfo.100 allows forward references.

Even if it is decided that this language does not allow forward refs in extensions or extended instruction sets, it doesn't seem to be a huge change to add a sentence here which would explicitly allow extensions and extended instruction sets to have forward refs.

So I will add to my suggestion:

  1. Add a sentence to SPIR-V core spec to allow forward references in extensions and extended instruction sets.
Keenuts commented 1 year ago

SPIR-V states the places forward references can appear:

OpenCL.DebugInfo.100 allows forward references.

This is, in the current state a "bug". Same reason we are now discussing over which method is better. It's not because it was done like this before that we should keep it in a half-valid state.

I would certainly prefer OpExtForwardReference to DebugTypeCompositeDeclare. But it seems a little more permissive and general than it needs to be

I don't require we make the simplest change, but my reference is not just allowing forward references in such a manner. I'd prefer B or F.

I would also be in favor of F:

F. New SPIR-V instruction like OpExtForwardReference to allow some IDs to be forward declared in related extended instructions.

To Greg's point "it seems a little more permissive". This instruction doesn't allow forward references in all places. It is just a SPRI-V wide, canonical method to say "in this instruction you don't know, you'll find forward references, act accordingly". But I still think extended instruction sets should by default refuse forward references, and allow then only if needed.

This way, from the extended spec point of view (knowing the context), the spec could say: For instruction X, Y can be a forward reference in case Z. But the general spec would also require the instruction X to be decorated/marked as containing forward references.

From a tools' perspective, they don't have to know the extended spec, only that "oh, instruction X is marked as having forward references, don't know why, but that's something to be careful about".

As I'm saying that, maybe it shouldn't be an instruction, but a decoration. So my current opinion is:

Let's make it an official SPIR-V decoration, which is applied to extended instruction having forward referenced.

I'll bring this to the WG 😊

Keenuts commented 1 year ago

Hello all!

This has been reviewed in the working group, and we now have a path forward!

First step: stop emitting bad instructions.

Compilers should not emit debug instruction with forward references. Since the current instruction set doesn't handle this, we should not emit debug instructions when they require forward references. In the HLSL case, this means missing some debug instructions for methods/objects. The compiler/tool should emit a warning saying "this is a known issue, cannot do it for now until it's fixed".

This will allow our tools to generate proper code, to the cost of a reduced debuggability.

Second step: add a new instruction/decoration to SPIR-V (OpExtForwardReference or similar)

We will start a proper review/ratification process for a new instruction/decoration to declare when a forward reference is allowed. This will take some time, hence the initial fix+warning in the meantime.

Keenuts commented 1 month ago

Hello all. The extension has now been merged on the SPIR-V side. Closing this since follow up will be on the tooling side now. Thanks all!

Validation rules added in SPIRV-Tools in https://github.com/KhronosGroup/SPIRV-Tools/pull/5698 Pass to fixup opcodes sent out: https://github.com/KhronosGroup/SPIRV-Tools/pull/5708 DXC issue to track the implementation in the compiler: https://github.com/microsoft/DirectXShaderCompiler/issues/6691