KhronosGroup / glslang

Khronos-reference front end for GLSL/ESSL, partial front end for HLSL, and a SPIR-V generator.
Other
2.91k stars 817 forks source link

default uniform block layout packing not work in HLSL. #3475

Open mrcino opened 6 months ago

mrcino commented 6 months ago

I'm using glslang to convert hlsl code to spir-v, but I found that some of the ubo members have incorrect offsets. I read part of the source code and found that ubo should use std140 layout by default, but it doesn't seem to take effect.

I made some modifications based on my understanding, and the rendering results alone are correct.

CLAassistant commented 6 months ago

CLA assistant check
All committers have signed the CLA.

mrcino commented 6 months ago

As additional information: I have a UBO which has the following members in hlsl:

{
   float4x4 _1;    // offset 0
   float4 _2;       // offset 64
   float _3;         // offset 80
   float2 _4;       // <====
}

In the std140 layout, member _4 should be aligned in base 8 bytes, so the offset should be 88; but the rendering result is incorrect;

After debugging with nsight, I found that this ubo was marked as scalar alignment, and the real offset is 84;

mrcino commented 5 months ago

Sorry…I think I messed something up…

I looked at a few failed test results and it seems like the test cases themselves need to be updated?

Please confirm, thank you

arcady-lunarg commented 5 months ago

I think you need to see whether the changes caused to the tests are reasonable and if so, update them. Are you sure your code is correct though? It seems like the merge was written that way for a specific reason, though I would have to dig into it a bit more to tell you whether it really works right.

mrcino commented 5 months ago

I think you need to see whether the changes caused to the tests are reasonable and if so, update them. Are you sure your code is correct though? It seems like the merge was written that way for a specific reason, though I would have to dig into it a bit more to tell you whether it really works right.

Thank you for your comment. In fact, I also feel that this change is a bit simple and crude. I will provide my process of troubleshooting this issue and hope to receive further advice from you.

I have noticed the purpose of this merge operation: it is specifically mentioned in the following code: image My initial assumption was probably incorrect...

When I found that the alignment of UBO did not match my expectations, I noticed that this situation occurred in the function 'HlslParseContext::fixBlockUniformOffsets' responsible for adjusting the offset of UBO members:

image

This causes alignment not to follow std140, and then I noticed the code here(in HlslParseContext::constructAggregate):

image

The direction of the merge here is a bit strange, because the default settings were not updated to type. getQualifier() before fixBlockUniformOffsets

The direction of the merge here is a bit strange, unless UBO defaults to scalar alignment when translating from hlsl to spir-v; If that's the case, my PR is indeed incorrect because I noticed that defaultQualification was indeed used later to update memberQualifier. If that's the case, I need to see if there are any methods for manually adjusting the layout;

carefully looked at it and I think the code sequence here is reversed: image I will make the necessary modifications before attempting again.

If you could provide a response to my question, I would greatly appreciate it.