[DXIL] Add -fhlsl-unused-resource-bindings=reserve-all to ensure consistent binding assignments

[Nielsbishere]

Adds a new compiler option: -fhlsl-unused-resource-bindings=[strip|reserve-all] .
strip keeps the current behavior.
reserve-all delays removing unused resources until after binding assignment, ensuring consistent register mappings across all entry points and library compilations when resources do not have explicit register annotations.

Under reserve-all, DCE removes only symbols initially, bindings are assigned with the complete resource list, and unused resources are stripped at the end. This produces deterministic and stable bindings without affecting optimization correctness.

Fixes #7931

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[llvm-beanz]

@hekota & @bogner, please look over this. I'm not sure how this aligns with our work in Clang and how we want to handle bindings in the future.

[Nielsbishere]

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.

[hekota]

@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 -consistent-bindings flag is compared to -keep-all-resources?

It seems to me that -keep-all-resources should already handle the consistent binding issue since it basically means “don’t strip unused resources.” The bindings in the example above end up inconsistent across different entry functions because the unused resources get stripped.

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.

[tex3d]

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.

[tex3d]

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.

[Nielsbishere]

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?

[Nielsbishere]

@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 compile process is quite complex, so I won't fully go over it here, but essentially:

• Legacy technique syntax is parsed and replaced (PSO combinations, entrypoint definitions); coming from a DX9 codebase to DXR/DX12.
• Shader parsing & preprocessing is ran to acquire register bindings & even optimized out registers (and to ensure consistent bindings manually by inserting register annotations)
• Compilation & linking (shader variants with IDxcLinker)
• Shader reflection is used on the final binary to find non optimized out resources (for transitions and knowing if certain debug commands are used)

The pipeline going forward could be:

• Legacy technique syntax the same way, or a different system managing this in C++
• Compile lib with -keep-all-resources to retrieve all resources
• Compile & link each unique define combination with -consistent-bindings (getting rid of optimized out resources, but having consistent bindings)
• Shader reflection (same thing as before) and validate that no new registers are created or have mismatching bindings for safety (though never happens in practice)

-keep-all-resources

is 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-bindings

is 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.
With both of those solved, the parser can drop all HLSL preprocessing and rely on C++ annotations for the remainder that can't easily be parsed. I'm not maintaining this system anymore, but in my own framework this would also be useful.

[Nielsbishere]

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?).
Basically what it does is it maintains all resources (except $Globals if no members or buffers of 0 size) and has a flag if the register is unused or not. Libraries use something similar but have more to worry about because one function might not reference a register but one of the others might.

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.

[hekota]

There is also the option to combine -keep-all-resources with -Qstrip_reflect for shipping binaries to make sure the implicit bindings are consistent and that there is no reflection bloat from it. Would that work in your case?

[Nielsbishere]

@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):

; Name                                 Type  Format         Dim      ID      HLSL Bind  Count
; ------------------------------ ---------- ------- ----------- ------- -------------- ------
;                                       UAV    byte         r/w      U0             u0     1
;                                       UAV    byte         r/w      U1             u1     1

This one still contains the unused resource. Or do you want me to modify Qstrip_reflect to drop unused resources?

[hekota]

Hi @Nielsbishere! You are very fast with the updates :)

We have realized we did not take into account the existing -flegacy-resource-reservation option in relation with these new flags. The legacy flag reserves the binding slots but only for explicitly bound resources, so it would be good to incorporate it into this new multi-value setting. We would have:

-fhlsl-unused-resource-bindings=strip             (default)
-fhlsl-unused-resource-bindings=reserve-explicit  (same as: -flegacy-resource-reservation)
-fhlsl-unused-resource-bindings=reserve-all       (was: -consistent-bindings)
-fhlsl-unused-resource-bindings=keep-all          (was: -keep-all-resources)

with the option to add keep-explicit option in the future if needed.

Could you please rename the values you are adding to reserve-all and keep-all? I can take care of adding reserve-explicit and mapping it to the -flegacy-resource-reservation later on.

[hekota]

Thank you for your contribution! As I mentioned above, we would like the options to
be called reserve-all and keep-all to enable adding additional options like reserve-explicit in the future.

If would be best if you could just add reserve-all value in this change, and once it is completed, you can update your other PR to add keep-all value to the enumeration. We generally want to avoid adding options that are not implemented or tested.

Also, the HLModule, DxilModule and CodeGenOptions should all use the UnusedResourceBindings enumeration instead of having separate flags for values (to avoid dealing with unsupported combinations of flags).

[Nielsbishere]

No problem, fixed it. I moved the enum to DxilConstants.h, not sure if that's the right place but seems like it's the easiest to access from all of these various places.
Instead of using Intermediate data I ended up adding it to the shader flags to ensure it lands into the final binary; this is required for the other PR since it needs to insert some shader variables to access the unbound resources in ID3D12FunctionReflection.

[hekota]

Looking good and getting close! Could you please update the title and description to reflect the latest changes since that will become the commit message?

@tex3d - could you please comment on whether DxilConstants.h is a good place for the new enumeration?

[hekota]

Love this!

[Nielsbishere]

@hekota updated it with new info, but I'm not sure if too verbose/it's too similar to an issue.
Do you want me to write it more similar to for example s-perron's style? (e.g .#7880)

[hekota]

@hekota updated it with new info, but I'm not sure if too verbose/it's too similar to an issue. Do you want me to write it more similar to for example s-perron's style? (e.g .#7880)

Commit messages are usually not as verbose as you have it now. We can shorten it when merging though. Usually there is an issue filed that describes the problem, and then a PR that fixes it with a short description on what was done, and which issue it resolves.

[tex3d]

I have requested several key changes:

• Don't use ShaderFlags for this, use HLSLOptions/IntermediateOptions instead.
• Don't bother removing from resource list in HLModule::RemoveGlobal, unconditionally set undef symbol instead.
• Don't condition calls to RemoveResourcesWithUnusedSymbols in DxilCondenseResources. Instead:
  • add bool AfterAllocation=false param
  • check AfterAllocation, UnusedResourceBinding, and LegacyResourceReservation (which will be folded into UnusedResourceBinding) for whether to remove resource
  • if not removing resource, set symbol to undef
• Coding standards / naming
• Need IR-based pass tests

[tex3d]

Need IR-based pass test to ensure intermediate options passed along.

[tex3d]

We need an IR-based pass test for changes that impact this pass.

[Nielsbishere]

Where do I add that? Any examples? (Same with the comment on DxilGenerationPass.cpp)

[tex3d]

Regarding these IR-based tests, there's a helper script: utils/hct/ExtractIRForPassTest.py -h for option help.
You pass in the desired pass and it stops at that point and generates the test output with the needed RUN line. You'll want to add -hlsl-dxilemit to the end of the pass list to emit the metadata.

However, as I was anticipating questions related to how to target this specific area, I tried crafting a test myself. Along the way, I ran into a bug in the pass, so I put a PR up to fix the bug, along with a couple pass tests.

Here's the PR: #7934
You could copy and modify the legalize-resource-phi.ll pass test for the purposes of testing changes to this hlsl-dxil-lower-handle-for-lib pass.

Notice how the metadata indicates unbound resources, then it checks the binding. For this change, this should check several things:

• The unused resources are removed from the resource list, like the example test already checks.
• The binding remains as-if the removed resources reserved registers (so u2 has binding 2 instead of 0 as this test currently checks).

[Nielsbishere]

Will do in a bit. Already pulled your PR into mine locally.

[Nielsbishere]

@tex3d I have a bit of trouble trying to wrap my head around this process. So if I simplify it in steps maybe you can see where it goes wrong.

1. python3 utils\hct\ExtractIRForPassTest.py D:\programming\repos\DirectXShaderCompiler\tools\clang\test\HLSLFileCheck\d3dreflect\consistent_bindings_simple.hlsl -p hlsl-dxil-lower-handle-for-lib -o test.txt -- -T cs_6_3 -E mainB -auto-binding-space 0 -fhlsl-unused-resource-bindings=reserve-all
2. Changing the RUN %dxopt line in the file it generated and adding -hlsl-dxilemit there like your example
3. Changing the UAV line/other checks to my example as well as the other checks to ensure it goes well?
4. Running the dxopt manually can allow me to see what I should be checking against/validate it goes well

So a few questions:

• I still get DI (debug info?) in my ll, as well as the dxc version info and resource info. How did you end up stripping that? Qstrip_debug doesn't work.
• Do I need to use the same HLSL file you generated the IL from so it behaves about the same? Or is the consistent bindings simple test fine as long as I keep similar checks as yours regarding the resources (e.g. check for u1 and u0 to be stripped)

[Nielsbishere]

@hekota Is the new description better? Also made an issue to match.