-
Notifications
You must be signed in to change notification settings - Fork 805
Generate consistent bindings #7643
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
base: main
Are you sure you want to change the base?
Generate consistent bindings #7643
Conversation
…he IDxcRewriter, this will allow generating register ids for registers that don't fully define them.
…ully qualified registers.
…allow passing different rewriter flags. Also added support for cbuffer auto bindings.
…dxil lowering properly, it's similar to -flegacy-resource-reservation but distinct in some key ways: the option only works for reserved registers and we need it to work on any register that has been compiled out. We also don't necessarily need the register to stay at the end of DXIL generation, in fact we'd rather not. Because having the register present can cause unintended performance penalties (for example, I use this reflection to know what transitions to issue)
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Important information to add is that the spirv backend already handles this correctly, so no fix is needed there. Will add a unit test as well. |
…g registers, $Globals, samplers, readonly, readwrite, etc.)
|
@Nielsbishere - Looking at this change and your proposed update to add -keep-all-resources flag, could you please help me understand what the benefit of the It seems to me that Also, as you’ve seen, implicit resource bindings tend to cause trouble since they make shader–application interfaces unpredictable and potentially compiler-dependent. It’d be great if you could find a way to stick with the explicit bindings to make sure your resource layouts stay stable and consistent. |
lib/HLSL/DxilCondenseResources.cpp
Outdated
| bChanged |= ResourceRegisterAllocator.AllocateRegisters(DM); | ||
|
|
||
| if (DM.GetConsistentBindings()) | ||
| DM.RemoveResourcesWithUnusedSymbols(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above, after the call to LegalizeResources, it calls RemoveResourcesWithUnusedSymbols if bLocalChanged. Don't you want to predicate that with if (!DM.GetConsistentBindings())? Otherwise, resources may be removed before calling AllocateRegisters.
Also, it seems we should be tracking changes from this with bChanged. bChanged = bChanged || (numResources != newResources); is used earlier for this, and a similar thing could be done here after updating newResources. An alternative is to modify RemoveResourcesWithUnusedSymbols to return true if any changes were made.
Side note: Looking a little further up, I noticed when ApplyBindingTableFromMetadata is called, it doesn't update bChanged either, but that's an existing bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need a test where LegalizeResources eliminates a resource use to test that code path. This relies on DxilValueCache, which was added largely to handle known-constant values in -Od cases where these values aren't simplified in the IR, but certain transformations we need to perform require these known values. In other words, you may be able to hit this code path using -Od with something that uses one resource or another in two code paths, depending on some condition that can be simplified to a constant, but is not normally simplified for -Od.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catches, have adjusted RemoveResourcesWithUnusedSymbols to return isModified and passing it along to bChanged. Also fixed the issue you said with LegalizeResources.
Since the issue ApplyBindingTableFromMetadata was already in there, it might be worth tackling this in a further PR/issue.
As for the test case for LegalizeResources; I'm not entirely sure where this would happen, I tried to read the code of LegalizeResources but it wasn't completely clear to me. Is there a unit test that tests legalize resources that I can combine with -consistent-bindings as a test case?
|
@hekota excuse me for the (following) long message, but these two have two very different uses in my previous work place. I'm not maintaining there anymore, but I still want them to have these changes to continue my work and will tell them how it can be used.
The pipeline going forward could be:
-keep-all-resourcesis intended to (currently not working as it should in reflection, due to a missing createHandle) allow behavior similar to -Od with spirv; keep all resources even if optimized out. The reason why you don't want this by default is because reflection (presence of a register) may be used to guide for example transitions (or to know which real registers to keep). Why would you even want optimized out registers? Because sometimes you want a consistent interface that knows about the inputs to a shader, even if those shaders don't use the resource. It can still optimize it in the background but the interface will still show this variable as present. An example could be a visual scripting language that relies on this. The one I'm talking about relies on manual shader parsing to find out optimized out registers for the stability, adding this feature would allow to remove the manual parsing eventually. Consistent bindings in comparison, doesn't affect the emitted registers, only the bindings. So -keep-all-resources could be used to discover reflection info first and then a final compile per entrypoint could be used with -consistent-bindings to ensure no registers will be leftover. -consistent-bindingsis a separate issue in the same system. In modern or legacy pipelines, shaders might have been written without explicit bindings or legacy semantics. These get registers auto assigned. Once again, the same shader parser is used to replace these legacy semantics with register bindings to avoid this issue this PR fixes. A legit use case for this is poor man's bindless, where you have arrays of textures/buffers. You could have 1 central include that defines this layout and puts them at space1 (no explicit bind point). If you change the first array size or add a register to this include it will automatically update. Otherwise you need to adjust all subsequent resource bindings. The system I'm talking about also validates that the final shader reflection matches the expected bindings to avoid accidental mismatches. |
…ymbols now return bool isModified. Passing isModified along from RemoveResourcesWithUnusedSymbols to bChanged and fixed the LegalizeResources + -consistent-bindings oversight.
|
I see what you mean though, I think this is very possible and I've added behavior to keep-all-resources that would make this possible, but it has a bug I'm trying to figure out now (duplicates functions somehow?). The big problem with that PR relative to the consistent bindings (when it works) is that I'm not sure how this would interact with any tools out in the wild. Anything that might assume registers will be stripped if they're unused would get issues if they see it, because they don't utilize this flag to detect if it's really unused or not. So personally I'd see it more as a 'give me more reflection' option rather than something people should ship binaries with until other tools/drivers are aware of this change in behavior. |
…operly trigger a modification
|
There is also the option to combine |
|
@hekota In theory yes, especially with my latest changes made that mark a resource with an unused flag to determine if a resource is used in the final binary. However, with -Qstrip_reflect It ends up only stripping the name for me (context: the simplified consistent bindings test from the other PR): This one still contains the unused resource. Or do you want me to modify Qstrip_reflect to drop unused resources? |
Currently, when resources are declared without explicit register bindings, DXC assigns them after
GlobalDCEruns. This can lead to inconsistent register assignments across entry points (depending on optimizations), which causes mismatched resource IDs.A common case is bindless-style includes, e.g. declaring multiple textures without explicit bindings but with spaces. In this situation, the generated binaries may not match across different entry points or when compiling as a library.
Solution
This PR introduces a new flag:
When enabled, DXC will generate stable bindings by letting
GlobalDCEeliminate only unused resources' symbols. Then, the bindings are generated and finally the unused resources are stripped again.This ensures consistent register assignments across all entry points, whether compiling as individual shaders or as a library.
-flegacy-resource-reservation, but tailored for ensuring binding consistency.Example
Command:
Shader:
Generated bindings:
a→t0b→t1c→u0Both
mainandmain2will consistently use the same bindings (batt1,catu0), regardless of entry point or library compilation.Notes