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

Feature request: reflect referenced members of a ByteAddressBuffer #234

Closed
danginsburg opened this issue Nov 6, 2023 · 8 comments · Fixed by #248
Closed

Feature request: reflect referenced members of a ByteAddressBuffer #234

danginsburg opened this issue Nov 6, 2023 · 8 comments · Fixed by #248
Assignees

Comments

@danginsburg
Copy link
Contributor

Given the following HLSL shader:

typedef uint Texture2DIndex_t;

struct MaterialData_t 
{
	float4 g_vTest;
	float2 g_vTest2;
	float3 g_vTest3;
	Texture2DIndex_t g_tTexture1;
	Texture2DIndex_t g_tTexture2;
	bool g_bTest1;
	bool g_bTest2;
};

static MaterialData_t _g_MaterialData;


ByteAddressBuffer g_MaterialData : register ( t4 , space1 )  ; 

struct Payload_t 
{ 
	float2 vTest;
	bool bTest;
	Texture2DIndex_t tTest;
	
}; 

struct PayloadShadow_t
{
	float m_flVisibility;
};

[ shader ( "closesthit" ) ] 
void ClosestHit0 ( inout Payload_t payload , in BuiltInTriangleIntersectionAttributes attrs ) 
{
	_g_MaterialData = g_MaterialData.Load<MaterialData_t>( InstanceIndex() );
 
    
    payload.vTest = _g_MaterialData.g_vTest2;
	payload.bTest = _g_MaterialData.g_bTest1;
	payload.tTest = _g_MaterialData.g_tTexture1;
	
    
} 

[ shader ( "anyhit" ) ] 
void AnyHit1 ( inout PayloadShadow_t payload , in BuiltInTriangleIntersectionAttributes attrs ) 
{
	_g_MaterialData = g_MaterialData.Load<MaterialData_t>( InstanceIndex() );
 
    { 
        payload . m_flVisibility = 0.0 ; 
        AcceptHitAndEndSearch ( ) ; 
    } 
} 

I would like to be able to use spirv-reflect to determine which members of g_MaterialData are statically referenced (the same as I would want to do for a cbuffer). Currently that is not possible.

This shader can be lowered to spir-v using slang with:

slangc.exe byte_address_buffer_reflection.hlsl -o byte_address_buffer_reflection_slang.spv -profile lib_6_4 -entry ClosestHit0

Or with DXC:

dxc.exe -spirv -T lib_6_4 byte_address_buffer_reflection.hlsl -fspv-target-env=vulkan1.2 -Fo byte_address_buffer_reflection_dxc.spv

I've attached the HLSL and slang and dxc-generated SPIR-V to the bug. Ideally spirv-reflect would work with shaders from either backend (DXC or slang).
byte_address_buffer_reflection.zip

@spencer-lunarg spencer-lunarg self-assigned this Nov 6, 2023
@spencer-lunarg spencer-lunarg added bug and removed bug labels Nov 7, 2023
@spencer-lunarg
Copy link
Contributor

So looking into this a few things

  1. I have noticed the only way I can ever tell this is the ByteAddressBuffer is because it is both an Input variable and the only thing with OpDecorate BuiltIn InstanceId ... I am not sure if this, along side the fact it is Ray Tracing stages, is enough to always know we should "start looking" for this

  2. The most information we are going to be able to get you is the offset into the struct that were accessed, from there you will need to map that to the original struct

  3. Where things get strange is the values I get for slang are [4, 5, 9, 11] but I get [4, 5, 11, 13] for dxc

the slang offsets makes sense

float4      g_vTest;     [0-3] 
float2      g_vTest2;    [4-5] // accessed
float3      g_vTest3;    [6-8]
uint        g_tTexture1; [9]   // accessed
uint        g_tTexture2; [10]  
bool        g_bTest1;    [11]  // accessed
bool        g_bTest2;    [12]

but for dxc I was only able to figure out there was some implicit padding from looking at the early IR (byte_address_hlsl_passes.txt)

... I don't know how to determine why it decided on adding the 2 bytes of padding here

float4      g_vTest;     [0-3]  
float2      g_vTest2;    [4-5]  // accessed
       PAD               [6-7]
float3      g_vTest3;    [8-10] 
uint        g_tTexture1; [11]   // accessed
uint        g_tTexture2; [12]   
bool        g_bTest1;    [13]   // accessed
bool        g_bTest2;    [14]

@danginsburg
Copy link
Contributor Author

Providing the offsets would be sufficient. The reason for the buffer offset differences is that I think by default DXC is using scalar block layout and slang is using std430. So it is correct that the offsets would be different. We have some compile option to use std430 layout for SSBOs on DXC so don't worry about those differences, it means it is doing the right thing. If you can report back the offset information I think I can work it out from there. Thanks!!

@spencer-lunarg
Copy link
Contributor

@danginsburg do you find the use of ByteAdressBuffer is only used with Ray Tracing stages or you feel this is something you would want for all stages?

@danginsburg
Copy link
Contributor Author

@danginsburg do you find the use of ByteAdressBuffer is only used with Ray Tracing stages or you feel this is something you would want for all stages?

The current need is for Ray Tracing stages, but ultimately it would be nice if it worked in all stages.

@spencer-lunarg
Copy link
Contributor

prototype #236

ok so spending more time into this, I realize the way slang does this is breaking all my assumption I had before from the DXC output

slang breaks the struct up into 3 variables (float only, uint only, and bool only) so you end up with 3 variables

the other issue is if there are ever 2 ByteAdresssBuffer in a shader, you won't have a good way (outside of hoping OpName is there) to match up which OpVariable points to which ByteAdresssBuffer

I will look into what it would take to add Non-semantic decorations we could put on the ByteAddressBuffer (maybe something we can add ontop of NonSemantic.Shader.DebugInfo)

@chaoticbob
Copy link
Contributor

the other issue is if there are ever 2 ByteAdresssBuffer in a shader, you won't have a good way (outside of hoping OpName is there) to match up which OpVariable points to which ByteAdresssBuffer

If the client side code knows what the corresponding binding / register are up to, it should be sufficient. If the use case is full reflection where the client code is building out descriptor set layouts, then you are correct - the information that SPIRV-Reflect provides will be underconstrained.

@spencer-lunarg
Copy link
Contributor

The issue is even if the client knows which binding/register, spirv-reflect will still have to attempt to find this information for every OpAccessChain which is very invasive, if it is known which variable is the ByteAddressBuffer it makes things more simpler and robust

@chaoticbob mentioned to me how some non-semantic info is optimized out (and experienced similar issue and hence why SPV_GOOGLE_user_type was created), but looking more, the SPV_KHR_non_semantic_info also doesn't get optimized away

the biggest issue I am finding and I am not sure how to make sure this info can be set down with both the DXC and slang front-end path

@chaoticbob
Copy link
Contributor

DXC already writes out the decorations for SPV_GOOGLE_user_type when -fspv-reflect is used. See below.

I would favor the SPV_GOOGLE_user_type approach for type decoration since that's what it was intended for.

Example shader:

ByteAddressBuffer   InData  : register(t0);
RWByteAddressBuffer OutData : register(u0);

[numthreads(1, 1, 1)]
void csmain(uint3 tid : SV_DispatchThreadID)
{
	OutData.Store(tid.x, InData.Load(tid.x));
}

Output:

$ dxc -spirv -fspv-reflect -T cs_6_0 -E csmain user_type.hlsl
; SPIR-V
; Version: 1.0
; Generator: Google spiregg; 0
; Bound: 25
; Schema: 0
               OpCapability Shader
               OpExtension "SPV_GOOGLE_hlsl_functionality1"
               OpExtension "SPV_GOOGLE_user_type"
               OpMemoryModel Logical GLSL450
               OpEntryPoint GLCompute %csmain "csmain" %gl_GlobalInvocationID
               OpExecutionMode %csmain LocalSize 1 1 1
               OpSource HLSL 600
               OpName %type_ByteAddressBuffer "type.ByteAddressBuffer"
               OpName %InData "InData"
               OpName %type_RWByteAddressBuffer "type.RWByteAddressBuffer"
               OpName %OutData "OutData"
               OpName %csmain "csmain"
               OpDecorate %gl_GlobalInvocationID BuiltIn GlobalInvocationId
               OpDecorateString %gl_GlobalInvocationID UserSemantic "SV_DispatchThreadID"
               OpDecorate %InData DescriptorSet 0
               OpDecorate %InData Binding 0
               OpDecorate %OutData DescriptorSet 0
               OpDecorate %OutData Binding 0
               OpDecorate %_runtimearr_uint ArrayStride 4
               OpMemberDecorate %type_ByteAddressBuffer 0 Offset 0
               OpMemberDecorate %type_ByteAddressBuffer 0 NonWritable

               OpDecorate %type_ByteAddressBuffer BufferBlock
               OpDecorateString %InData UserTypeGOOGLE "byteaddressbuffer"
               OpMemberDecorate %type_RWByteAddressBuffer 0 Offset 0
               OpDecorate %type_RWByteAddressBuffer BufferBlock
               OpDecorateString %OutData UserTypeGOOGLE "rwbyteaddressbuffer"

       %uint = OpTypeInt 32 0
     %uint_2 = OpConstant %uint 2
     %uint_0 = OpConstant %uint 0
%_runtimearr_uint = OpTypeRuntimeArray %uint
%type_ByteAddressBuffer = OpTypeStruct %_runtimearr_uint
%_ptr_Uniform_type_ByteAddressBuffer = OpTypePointer Uniform %type_ByteAddressBuffer
%type_RWByteAddressBuffer = OpTypeStruct %_runtimearr_uint
%_ptr_Uniform_type_RWByteAddressBuffer = OpTypePointer Uniform %type_RWByteAddressBuffer
     %v3uint = OpTypeVector %uint 3
%_ptr_Input_v3uint = OpTypePointer Input %v3uint
       %void = OpTypeVoid
         %16 = OpTypeFunction %void
%_ptr_Uniform_uint = OpTypePointer Uniform %uint
     %InData = OpVariable %_ptr_Uniform_type_ByteAddressBuffer Uniform
    %OutData = OpVariable %_ptr_Uniform_type_RWByteAddressBuffer Uniform
%gl_GlobalInvocationID = OpVariable %_ptr_Input_v3uint Input
     %csmain = OpFunction %void None %16
         %18 = OpLabel
         %19 = OpLoad %v3uint %gl_GlobalInvocationID
         %20 = OpCompositeExtract %uint %19 0
         %21 = OpShiftRightLogical %uint %20 %uint_2
         %22 = OpAccessChain %_ptr_Uniform_uint %InData %uint_0 %21
         %23 = OpLoad %uint %22
         %24 = OpAccessChain %_ptr_Uniform_uint %OutData %uint_0 %21
               OpStore %24 %23
               OpReturn
               OpFunctionEnd

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

Successfully merging a pull request may close this issue.

3 participants