Skip to content

AggressiveDCE: Retain all bindings#2678

Closed
troughton wants to merge 1 commit intoKhronosGroup:masterfrom
troughton:patch-1
Closed

AggressiveDCE: Retain all bindings#2678
troughton wants to merge 1 commit intoKhronosGroup:masterfrom
troughton:patch-1

Conversation

@troughton
Copy link
Copy Markdown
Contributor

Some background: my shader render pipeline uses SPIRV-Cross to convert SPIR-V into Metal shaders. For resource binding, argument buffers are used, mapping directly to Vulkan descriptor sets. Crucially, these argument buffers are shared across pipeline stages; each argument buffer may contain resources relevant to both the vertex and fragment stage.

As a result of this, I need all bindings to be visible to all shader stages, even if the binding may be unused in that stage: the argument buffer definition must be complete. To achieve this, the AggressiveDCE pass must not strip instructions with binding decorations. (Additional changes are also needed for SPIRV-Cross to support this; see troughton/SPIRV-Cross@c3daef7)

Note that simply not running the pass isn't an option; my source shaders are in HLSL so need to undergo legalisation.

I don't expect this to be merged as-is (since that would obviously be undesirable for code-size in a number of shaders), but I welcome suggestions for how retaining bindings should be added as an option – should it be added an option to AggressiveDCE, or should a separate pass be used? How should the option be exposed in the driver?

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Jun 18, 2019

CLA assistant check
All committers have signed the CLA.

@alan-baker alan-baker requested a review from s-perron June 18, 2019 12:50
@s-perron
Copy link
Copy Markdown
Collaborator

Sorry, I though I had responded earlier. For the design, I would suggest adding a flag that is passed as a parameter to the constructor for AggressiveDCEPass. Your code should be guarded by that flag.

For the public interface, CreateAggressiveDCEPass should take the flag as a parameter as well, and it should default to the original behaviour. We do not want to force others to have to change their code.

Lastly add a new suboption to eliminate-dead-code-aggressive. See scalar replacement for how to do that. Also, update the documentation for the option in tools/opt/opt.cpp.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants