NonSemantic.Shader.DebugInfo.100 & forward references #203
Comments
|
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. |
|
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. |
|
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 Member functions and types would work similarly. |
|
There is a related issue in glslang as well, where it is not handling forward references when generating DebugInfo: KhronosGroup/glslang#3189 |
|
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 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? |
|
Thanks for all the answers 😊
Or remove the method definition from the member list, as we do in spirv?
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). What seems to be true is what the spec says about non-semantic: 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? |
|
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 |
|
Structs in SPIR-V do not allow forward references. It is assumed that any cycle in the types can be solved using |
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 Only debug instruction defines them in the struct instruction so there is a link between the struct definition and the function definition. 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? |
@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? |
|
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 ( 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
OpFunctionEndand with debug info: ( 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
OpFunctionEndAs you can see, the SPIR-V way to handle methods is to just give a 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_2The Right now, those instruction are defined as follows: 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.
This would allow us to remove forward references. |
|
@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. |
|
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. |
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. |
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
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: If we want to do it at the non-semantic debug level, it would require adding the following:
Usages would be as follows: 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
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. |
My thought would be more like we'd have to change the debug instruction set to avoid it. Something like: 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.
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. |
So this would invert the DebugTypeMember usage we have today, and require an additional type definition, but could work yes: would be represented as 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 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. |
|
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. |
|
Hello! So far, we have a few proposed ideas: A. Change
|
|
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. |
|
Wouldn't option
|
|
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: but I must acknowledge that the solution for this was a bit hacky and we left some 'dead' instructions on the way. |
|
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. |
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. |
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 |
@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:
Notice no changes to SPIR-V core or NonSemantic extension are proposed or needed. |
SPIR-V states the places forward references can appear:
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:
|
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 also be in favor of 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". This way, from the extended spec point of view (knowing the context), the spec could say: 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. 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 😊 |
|
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. 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 (
|
When generating debug instructions for a function, each one is linked to its scope. In case of member functions, this scope is the class. When declaring a class, all its member, including functions must be declared. This cycle requires a forward reference, which is allowed by the debug instruction spec, but not by parents: NonSemantic & SPIR-V specs. This is a spec issue we have to fix. In the meantime, we decided to emit a warning, and generate slightly worse debug instructions. Context: KhronosGroup/SPIRV-Registry#203 Signed-off-by: Nathan Gauër <[email protected]>
When generating debug instructions for a function, each one is linked to its scope. In case of member functions, this scope is the class. When declaring a class, all its member, including functions must be declared. This cycle requires a forward reference, which is allowed by the debug instruction spec, but not by parents: NonSemantic & SPIR-V specs. This is a spec issue we have to fix. In the meantime, we decided to emit a warning, and generate slightly worse debug instructions. Context: KhronosGroup/SPIRV-Registry#203 Signed-off-by: Nathan Gauër <[email protected]>
When generating debug instructions for a function, each one is linked to its scope. In case of member functions, this scope is the class. When declaring a class, all its member, including functions must be declared. This cycle requires a forward reference, which is allowed by the debug instruction spec, but not by parents: NonSemantic & SPIR-V specs. This is a spec issue we have to fix. In the meantime, we decided to emit a warning, and generate slightly worse debug instructions. Context: KhronosGroup/SPIRV-Registry#203 Signed-off-by: Nathan Gauër <[email protected]>
When generating debug instructions for a function, each one is linked to its scope. In case of member functions, this scope is the class. When declaring a class, all its member, including functions must be declared. This cycle requires a forward reference, which is allowed by the debug instruction spec, but not by parents: NonSemantic & SPIR-V specs. This is a spec issue we have to fix. In the meantime, we decided to emit a warning, and generate slightly worse debug instructions. Context: KhronosGroup/SPIRV-Registry#203 Signed-off-by: Nathan Gauër <[email protected]>
When generating debug instructions for a function, each one is linked to its scope. In case of member functions, this scope is the class. When declaring a class, all its member, including functions must be declared. This cycle requires a forward reference, which is allowed by the debug instruction spec, but not by parents: NonSemantic & SPIR-V specs. This is a spec issue we have to fix. In the meantime, we decided to emit a warning, and generate slightly worse debug instructions. Context: KhronosGroup/SPIRV-Registry#203 --------- Signed-off-by: Nathan Gauër <[email protected]>
|
Hello all. Validation rules added in SPIRV-Tools in KhronosGroup/SPIRV-Tools#5698 |
The NonSemantic.Shader.DebugInfo.100 has the following generic statement:
However, later while defining the DebugTypeComposite instruction, it says:
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.
The text was updated successfully, but these errors were encountered: