Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Workaround AccessChain function arguments for DebugDeclare #5275

Closed

Conversation

jeremy-lunarg
Copy link
Contributor

@jeremy-lunarg jeremy-lunarg commented Jun 16, 2023

Fix DXC issue #5191.

Substitute variables for access chain function arguments. For each function argument that is an access chain, create a new variable, copy the access chain pointee into the variable before the function call, substitute the variable in the function call, and copy the variable back into the access chain pointee after the function call.

This is a workaround for NonSemantic.Shader.DebugInfo.100. DebugDeclare expects the operand variable to be a result id of an OpVariable or OpFunctionParameter. However, function arguments may contain OpAccessChains during legalization which causes problems for DebugDeclare.

Substitute variables for access chain function arguments. For each
function argument that is an access chain, create a new variable, copy
the access chain pointee into the variable before the function call,
substitute the variable in the function call, and copy the variable back
into the access chain pointee after the function call.

This is a workaround for NonSemantic.Shader.DebugInfo.100. DebugDeclare
expects the operand variable to be a result id of an OpVariable or
OpFunctionParameter. However, function arguments may contain
OpAccessChains during legalization which causes problems for
DebugDeclare.
@jeremy-lunarg jeremy-lunarg requested a review from s-perron June 16, 2023 22:51
@jeremy-lunarg jeremy-lunarg self-assigned this Jun 16, 2023
@jeremy-lunarg
Copy link
Contributor Author

@s-perron I've been picking @greg-lunarg's brain for ideas to fix this bug and this is the approach we arrived at. Can you take a look and let us know what you think?

@jeremy-lunarg jeremy-lunarg changed the title Fix 5191 WIP: Workaround AccessChain function arguments for DebugDeclare Jun 16, 2023
@s-perron
Copy link
Collaborator

Thanks for asking. I do not believe that this solution is correct. The problem is that passing in the address of a different variable and doing a copy-in/copy-out is different than passing in the address of a variable. You can see microsoft/DirectXShaderCompiler#5158 for a discussion on how this could make a difference.

Could you use Indexes in the debug declare to represent the variable? If parameter is a series of OpAccessChains, then the debug declare that is generated should contains the base variable for the Variable parameter, and all of the indexes in the OpAccessChains for the Indexes.

@s-perron
Copy link
Collaborator

Here is a specific example. I may also show some other difficulties with your solution. If you change the parameter to be a pointer to a function scope variable, then the store to data will not be visible because the shader will terminate before foo returns, and the "copy-out" will not happen.

The other problem (potentially I did not read your code too carefully) is that the access chain in foo must get it storage class changed to Function if you modify the pointer that is used as a parameter.

; SPIR-V
; Version: 1.6
; Generator: Khronos SPIR-V Tools Assembler; 0
; Bound: 26
; Schema: 0
               OpCapability Shader
               OpCapability VariablePointersStorageBuffer
          %1 = OpExtInstImport "GLSL.std.450"
               OpMemoryModel Logical GLSL450
               OpEntryPoint Fragment %main "main" %_
               OpExecutionMode %main OriginUpperLeft
               OpSource GLSL 460
               OpSourceExtension "GL_GOOGLE_cpp_style_line_directive"
               OpSourceExtension "GL_GOOGLE_include_directive"
               OpName %main "main"
               OpName %foo_f1_ "foo(f1;"
               OpName %data "data"
               OpName %SSBO "SSBO"
               OpMemberName %SSBO 0 "data"
               OpName %_ ""
               OpDecorate %_arr_float_uint_100 ArrayStride 4
               OpDecorate %_arr__arr_float_uint_100_uint_100 ArrayStride 400
               OpMemberDecorate %SSBO 0 Offset 0
               OpDecorate %_ DescriptorSet 0
               OpDecorate %_ Binding 0
       %void = OpTypeVoid
         %10 = OpTypeFunction %void
      %float = OpTypeFloat 32
%_ptr_StorageBuffer_float = OpTypePointer StorageBuffer %float
       %uint = OpTypeInt 32 0
     %uint_0 = OpConstant %uint 0
     %uint_1 = OpConstant %uint 1
   %uint_100 = OpConstant %uint 100
    %float_5 = OpConstant %float 5
%_arr_float_uint_100 = OpTypeArray %float %uint_100
%_arr__arr_float_uint_100_uint_100 = OpTypeArray %_arr_float_uint_100 %uint_100
%_ptr_StorageBuffer__arr_float_uint_100 = OpTypePointer StorageBuffer %_arr_float_uint_100
         %19 = OpTypeFunction %void %_ptr_StorageBuffer__arr_float_uint_100
       %SSBO = OpTypeStruct %_arr__arr_float_uint_100_uint_100
%_ptr_StorageBuffer_SSBO = OpTypePointer StorageBuffer %SSBO
          %_ = OpVariable %_ptr_StorageBuffer_SSBO StorageBuffer
       %main = OpFunction %void None %10
         %21 = OpLabel
         %22 = OpAccessChain %_ptr_StorageBuffer__arr_float_uint_100 %_ %uint_0 %uint_1
         %23 = OpFunctionCall %void %foo_f1_ %22
               OpReturn
               OpFunctionEnd
    %foo_f1_ = OpFunction %void None %19
       %data = OpFunctionParameter %_ptr_StorageBuffer__arr_float_uint_100
         %24 = OpLabel
         %25 = OpAccessChain %_ptr_StorageBuffer_float %data %uint_0
               OpStore %25 %float_5
               OpTerminateInvocation
               OpFunctionEnd

@jeremy-lunarg
Copy link
Contributor Author

jeremy-lunarg commented Jun 22, 2023

Thanks @s-perron. I've taken a look at DXC #5158. I agree that fixing this issue at the root is a better solution. @llvm-beanz has begun this work on this branch.

@BattleAxeVR
Copy link

Thanks @s-perron. I've taken a look at DXC #5158. I agree that fixing this issue at the root is a better solution. @llvm-beanz has begun this work on this branch.

Hi, I'm one of the two people who have this issue in the AnyHit shader, trying to output variables to the ray payload when the ray is stopped (so the caller can do something with data stored in the payload).

It looks like work on that branch stopped two months ago, was the actual fix implemented? It's unclear what the outcome of all this is at this point. Thanks

@s-perron
Copy link
Collaborator

@BattleAxeVR This would not have fixed your problem. This PR would have tried to fix the error caused by inlining. It would have created a new temporary variable, and it would have cause the payload to not be written to until we returned from the function.

The solution for your problem will be the implementation of the c++ references. I'm not sure how far away that is. microsoft/DirectXShaderCompiler#5675 might be able to help you as well, but that will not be official, and not supported by the DXC maintainers.

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