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

[reflection-api] How to know if TypeLayout has scalar layout ? #5715

Closed
obhi-d opened this issue Dec 1, 2024 · 13 comments · Fixed by #5797
Closed

[reflection-api] How to know if TypeLayout has scalar layout ? #5715

obhi-d opened this issue Dec 1, 2024 · 13 comments · Fixed by #5797
Assignees
Labels
goal:client support Feature or fix needed for a current slang user. goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang

Comments

@obhi-d
Copy link
Contributor

obhi-d commented Dec 1, 2024

Hi,

In my shader example : Shader

When I use the reflection API to give me the offsets for the MaterialData structure, which is passed as a pointer to the vertexMain function, I use this method:

slang::TypeLayoutReflection* typeLayout; // points to MaterialData
bufferSize = typeLayout->getSize();
// Validate category for each member, and get offset
...
if (category == SLANG_PARAMETER_CATEGORY_UNIFORM)
 member.offset = var->getOffset(SLANG_PARAMETER_CATEGORY_UNIFORM);

Currently, the issue is, while I go inside the typeLayout of the underlying type of the pointer type that is passed to vertexMain, as I ask the offset and size with category set as UNIFORM, I get offset which are completely different and aligned to 16 byte for example, than the actual offset of the SPIRV offset set for the individual member as you can see in the output.
Currently I am also stuck with another bug where going through the non-bindless path does not produce correct SPIRV, and duplicates descriptor ranges, which I already reported, so I am unable to verify if the offsets I obtain would be correct, had this buffer been bound via a ParameterGroup instead of a pointer.

Thanks in advance for pointing me to the right direction.

@csyonghe
Copy link
Collaborator

csyonghe commented Dec 3, 2024

It seems to me that the layout you are querying are obtained using std140 rules (aggresively aligning things to 16 bytes), while the actual data should be laid out according to std430 rules. How did you obtain typeLayout?

I think there is a gap in the reflection API. Basically you want to call

ShaderReflection* r;

auto type = r->findTypeByName("MaterialData");
auto typeLayout = r->getTypeLayout(type, SLANG_LAYOUT_RULES_DEFAULT_STORAGE_BUFFER); // use storage buffer rules to obtain the layout.

Unfortunately we didn't expose SLANG_LAYOUT_RULES_DEFAULT_STORAGE_BUFFER yet.

@obhi-d
Copy link
Contributor Author

obhi-d commented Dec 3, 2024

In-fact, I am just iterating over my function parameters by entryPoint reflection like so-

auto parameterCount       = entryPoint->getParameterCount();
      for (uint32_t p = 0; p < parameterCount; ++p)
      {
        auto param = entryPoint->getParameterByIndex(p);
        emitParameter(param);
      }

In emitParameter I just check the layout of the pointer, then I obtain the ElementTypeLayout of the param's typeLayout.
My code is fairly generic and tries not to assume anything by type names.
I suppose the element type layout of a pointer which is a push constant variable should not be std140/430 layout, so it could be a bug?

@obhi-d
Copy link
Contributor Author

obhi-d commented Dec 3, 2024

Another point, the buffer is packed with scalar layout, not as std430, which I am completely okay with

@bmillsNV
Copy link
Collaborator

bmillsNV commented Dec 5, 2024

@obhi-d is this issue currently blocking you or are you OK if we investigate in Q1 of 2025?

@bmillsNV bmillsNV added the Needs reporter feedback Bugs awaiting more information from the reporter label Dec 5, 2024
@obhi-d
Copy link
Contributor Author

obhi-d commented Dec 6, 2024

This is a blocking issue for me, because I do not have a way to query the layout chosen by Slang. I also dont see how reflection interface can be used correctly without having a way to get the correct offsets. It feels like I can only get field names out of reflection and be content with it, and calculate everything else.
EDIT: I don't mind this being investigated in Q1 2025

@csyonghe
Copy link
Collaborator

csyonghe commented Dec 6, 2024

I need some more context understanding why there is a need to figure out if an existing layout is std140 or std430.

Most of the time the layout is well defined: StructuredBuffer element type is always std430 by default, and push constant is always std430 by default, and constant buffer is always std140 by default.

Slang also allows you to explicitly specify the layout to use for each individual buffer, by using ConstantBuffer<T, Std430DataLayout> or StructuredBuffer<T, ScalarDataLayout>.

You should also be able to query the offset of each field in a ConstantBuffer using the reflection API.

@csyonghe
Copy link
Collaborator

csyonghe commented Dec 6, 2024

The default layout can also be changed with -force-glsl-scalar-layout or -fvk-use-dx-layout compiler options.

@csyonghe
Copy link
Collaborator

csyonghe commented Dec 6, 2024

Generally, it is not meaningful to ask the offset of T::member without an explicit layout rule. If you can provide the detailed code that you are using to obtain the TypeLayout for the uniform data in question, we may be able to diagnose more into what was wrong with the reflection logic.

@obhi-d
Copy link
Contributor Author

obhi-d commented Dec 6, 2024

Here is my attached source if you want to have a look, function where I start iterating over parameter is emitParameter
SlangModuleCompiler.cpp.txt

I will probably assume the struct layout (scalar or std430) from parameter type, and force slang to use specific layouts as a work-around. I will provide you with more details later in the day.

@obhi-d
Copy link
Contributor Author

obhi-d commented Dec 6, 2024

Will it be possible to expand LayoutRules to StructureBuffer and ConstantBuffer? The problem for me has been to get field offsets for a structure buffer, passed as an pointer in push constants, in my engine's bindless path. In non bindless path, this is a constant buffer with a binding generated by slang. In either case, the code needs to read all field info and fill the ShaderProgram instance with the shadar parameter names and offsets.

@csyonghe
Copy link
Collaborator

csyonghe commented Dec 6, 2024

Yes, this should be easy to add.

@csyonghe
Copy link
Collaborator

csyonghe commented Dec 7, 2024

I fixed the layout logic for pointer element types in PR #5797, you can refer to the test case added here: https://github.com/shader-slang/slang/blob/a3ec188b1bae46d0390b71f88e931946c4ced187/tools/slang-unit-test/unit-test-ptr-layout.cpp

Basically, the offsets you query from a TypeLayout("Ptr<StructType>")->getElementTypeLayout() should now be correct.

@csyonghe csyonghe added goal:client support Feature or fix needed for a current slang user. goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang and removed Needs reporter feedback Bugs awaiting more information from the reporter labels Dec 7, 2024
@csyonghe csyonghe added this to the Q4 2024 (Fall) milestone Dec 7, 2024
@obhi-d
Copy link
Contributor Author

obhi-d commented Dec 7, 2024

Thanks a lot for the quick fix !

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. goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants