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

Support -fvk-use-dx-layout. #4126

Closed
csyonghe opened this issue May 7, 2024 · 5 comments · Fixed by #4912
Closed

Support -fvk-use-dx-layout. #4126

csyonghe opened this issue May 7, 2024 · 5 comments · Fixed by #4912
Assignees
Labels
goal:client support Feature or fix needed for a current slang user. kind:enhancement a desirable new feature, option, or behavior

Comments

@csyonghe
Copy link
Collaborator

csyonghe commented May 7, 2024

We already have -force-glsl-scalar-layout that causes all buffers to follow the scalar layout in the resulting spirv. We also need -fvk-use-dx-layout to make them follow the dx layout.

@csyonghe
Copy link
Collaborator Author

csyonghe commented May 8, 2024

From Hai:

Here's all the resources that I could scrounge up for this.

For DXC, the selection for the layout rules begins here based on what options were passed in:
https://github.com/microsoft/DirectXShaderCompiler/blob/main/tools/clang/lib/SPIRV/SpirvEmitter.cpp#L605-L609

Looking through this reminded me it was actually the FXC rules that we had to follow, so that's why the enums are prefixed with Fxc.
cbuffer/ConstantBuffer/tbuffers all use the FxcCTBuffer rules.
StructuredBuffer/RWStructuredBuffer + all their variants use the FxcSBuffer rules.
Amplification shader payloads also use FxcSBuffer rules.

The rules (FxcCTBuffer and FxcSBuffer) are spelled out and implemented in this file:
https://github.com/microsoft/DirectXShaderCompiler/blob/main/tools/clang/lib/SPIRV/AlignmentSizeCalculator.cpp#L245-L261

DXC's documentation also spells out these rules although it takes a bit parsing:
https://github.com/microsoft/DirectXShaderCompiler/wiki/Buffer-Packing

Microsoft's documentation on HLSL packing rules is pretty simplistic:
https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-packing-rules

This is a pretty awesome visualizer that someone wrote to illustrate the HLSL Constant Buffer Packing Rules:
https://maraneshi.github.io/HLSL-ConstantBufferLayoutVisualizer/

@bmillsNV bmillsNV added this to the Q2 2024 (Spring) milestone May 9, 2024
@bmillsNV bmillsNV added kind:enhancement a desirable new feature, option, or behavior goal:client support Feature or fix needed for a current slang user. labels May 9, 2024
@bmillsNV
Copy link
Collaborator

bmillsNV commented May 9, 2024

Not sure if we've got capacity for this one in Q2 or not. Marking for Q2 for now. Need to do another scrub of issues in Q2 to see how this one ranks.

@bmillsNV
Copy link
Collaborator

Marking for Q3. Planning to tackle this request in July to support Siggraph.

@bmillsNV
Copy link
Collaborator

@ArielG-NV can you take a look at this one?

@ArielG-NV
Copy link
Contributor

I can take a look at this issue

ArielG-NV added a commit to ArielG-NV/slang that referenced this issue Aug 23, 2024
Fixes: shader-slang#4126

Changes:
* Added fvk-use-dx-layout
* Modified `HLSLConstantBufferLayoutRulesImpl` for correctness (ex: Array is always 16 byte aligned)
* Added kFXCShaderResourceLayoutRulesFamilyImpl and kFXCConstantBufferLayoutRulesFamilyImpl to handle fvk-use-dx-layout
* Added `ConstantBufferLayoutRules` to manage constant buffer rules
* Added `alignCompositeElementOfNonAggregate`/`alignCompositeElementOfAggregate` to handle forced alignment of composites for ConstantBuffers
* `StructuredBuffer` rules are mostly equal to `scalar` layout, not much was needed to be changed to support this behavior.
ArielG-NV added a commit to ArielG-NV/slang that referenced this issue Aug 23, 2024
Fixes: shader-slang#4126

Changes:
* Added fvk-use-dx-layout
* Modified `HLSLConstantBufferLayoutRulesImpl` for correctness (ex: Array is always 16 byte aligned)
* Added kFXCShaderResourceLayoutRulesFamilyImpl and kFXCConstantBufferLayoutRulesFamilyImpl to handle fvk-use-dx-layout
* Added `ConstantBufferLayoutRules` to manage constant buffer rules
* Added `alignCompositeElementOfNonAggregate`/`alignCompositeElementOfAggregate` to handle forced alignment of composites for ConstantBuffers
* `StructuredBuffer` rules are mostly equal to `scalar` layout, not much was needed to be changed to support this behavior.
ArielG-NV added a commit to ArielG-NV/slang that referenced this issue Aug 23, 2024
Fixes: shader-slang#4126

Changes:
* Added fvk-use-dx-layout
* Modified `HLSLConstantBufferLayoutRulesImpl` for correctness (ex: Array is always 16 byte aligned)
* Added kFXCShaderResourceLayoutRulesFamilyImpl and kFXCConstantBufferLayoutRulesFamilyImpl to handle fvk-use-dx-layout
* Added `ConstantBufferLayoutRules` to manage constant buffer rules
* Added `alignCompositeElementOfNonAggregate`/`alignCompositeElementOfAggregate` to handle forced alignment of composites for ConstantBuffers
* `StructuredBuffer` rules are mostly equal to `scalar` layout, not much was needed to be changed to support this behavior.
csyonghe added a commit that referenced this issue Aug 26, 2024
* Implement `-fvk-use-dx-layout`

Fixes: #4126

Changes:
* Added fvk-use-dx-layout
* Modified `HLSLConstantBufferLayoutRulesImpl` for correctness (ex: Array is always 16 byte aligned)
* Added kFXCShaderResourceLayoutRulesFamilyImpl and kFXCConstantBufferLayoutRulesFamilyImpl to handle fvk-use-dx-layout
* Added `ConstantBufferLayoutRules` to manage constant buffer rules
* Added `alignCompositeElementOfNonAggregate`/`alignCompositeElementOfAggregate` to handle forced alignment of composites for ConstantBuffers
* `StructuredBuffer` rules are mostly equal to `scalar` layout, not much was needed to be changed to support this behavior.

* seperate legacy constant buffer and how Slang does constant-buffer normally

* undo an addition

* remove accidental test

* Address review and fix

Address review and remove GLSL support since GLSL requires a seperate legalization (need to linearlize structs like with `legalizeMetalIR` to assign explicit offsets)

* comments

* remove aggregate and non-aggregate logic

We don't need this distinction for the logic

---------

Co-authored-by: Yong He <yonghe@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
goal:client support Feature or fix needed for a current slang user. kind:enhancement a desirable new feature, option, or behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants