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

Improve code generated for node record .Get() #256

Open
tcorringham opened this issue Jun 11, 2024 · 2 comments
Open

Improve code generated for node record .Get() #256

tcorringham opened this issue Jun 11, 2024 · 2 comments
Labels
enhancement New feature or request needs-triage
Milestone

Comments

@tcorringham
Copy link

In node shaders that uses an input record the .Get() access mechanism is a bit clumsy:

 Output[gid] = inputRecord.Get().data1[gid];

and it is tempting to use a local variable to write code like:

 const RecordType record = inputRecord.Get();
 Output[gid] = record.data1[gid];
 ...

In both cases any fields in the record that are not referenced are not loaded.
In the first version, only the single array element indexed (by variable gid) is loaded.
However, in the second version all elements of the array are loaded (unless the array index happens to have a known value).
This can lead to unexpected and surprising runtime overheads, especially when converting existing code to use work graphs.

Describe the solution you'd like
The proposed '->' operator would allow more natural code of the form:

Output[gid] = inputRecord->data1[gid];

Alternatively, where there are only invariant indices used (possibly where only a single value is used as an index), only those need be loaded - effectively deferring the generation of the loads until the usage is known just as if the Get() had been used as in the first example.

Describe alternatives you've considered
The use of a macro is an alternative to the first example, but just as ugly.

Additional context
Example on gobolt : https://godbolt.org/z/bM6nGh1eo

@tcorringham tcorringham added enhancement New feature or request needs-triage labels Jun 11, 2024
@llvm-beanz
Copy link
Collaborator

This adds new syntax to HLSL, so this is a language change and needs to be treated as such. Sending to hlsl-specs.

@llvm-beanz llvm-beanz transferred this issue from microsoft/DirectXShaderCompiler Jun 11, 2024
@llvm-beanz
Copy link
Collaborator

This feature is likely intwined with the larger feature to add reference support to HLSL (https://github.com/microsoft/hlsl-specs/blob/main/proposals/0006-reference-types.md).

@llvm-beanz llvm-beanz added this to the HLSL Backlog milestone Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-triage
Projects
Status: Triaged
Development

No branches or pull requests

2 participants