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

Add CLI option to set CreateAggressiveDCEPass(true) #5522

Closed
alecazam opened this issue Jan 10, 2024 · 7 comments · Fixed by #5524
Closed

Add CLI option to set CreateAggressiveDCEPass(true) #5522

alecazam opened this issue Jan 10, 2024 · 7 comments · Fixed by #5524
Assignees

Comments

@alecazam
Copy link

alecazam commented Jan 10, 2024

The change broke our HLSL codegen where preserve_interface on this is hardcoded to false. There is no CLI option to alter this. More details here:

KhronosGroup/Vulkan-Docs#666

We'd like to move to tools past this change back in 2021. Stripping inputs from the entry function may work fine for Vulkan spirv. For transpiling to HLSL, this change broke our PS4 shader pipeline which uses spirv-opt to eliminate various fields. spirv-opt has no idea that optimized spirv will be transpiled to HLSL to set the flag to true on it's ADCE passes. Also, the input gaps formed by this aren't handled by the HLSL/PSSL compilers that we use. Please add a command line option to restore the original behavior.

This preserves the entry-point interfaces and variables.
--eliminate-dead-code-aggressive-hlsl

Adds CreateAggressiveDCEPass(true)

I found someone that added a flag to pass the preserve_interface flag across the optimize and legalize options, but then never seemed to bother setting it to true. Alternatively a flag could set preserve_interfaces to true across all uses.

#5268

We just run the tools out of the Vulkan SDK, we don't call APIs or build from source. So we need a command line option to get the old behavior. This had to break Roblox code too, so I don't see how this ended up in the main codebase.

@alecazam alecazam changed the title Change to set preserve_interface to false on ADCE broke HLSL transpiling. Add CLI option to set CreateAggressiveDCEPass(true) Jan 10, 2024
@s-perron s-perron self-assigned this Jan 10, 2024
@s-perron
Copy link
Collaborator

As in previous discussion about this topic, spirv-opt is an optimizer for SPIR-V. When the input is a shader, we assume we are targeting Vulkan. That assumption will always be the default, and what we will be primarily testing. However, we are willing to add flags that will enable other work flows.

I'll have to think about how to make the command line interface intuitive to enable your work flow. We will need changes to multiple options: -O, -Os, and --eliminate-dead-code-aggressive. I don't want the interface to mention HLSL because it is not really about targeting HLSL. I'll want it to be about targeting the interface variables.

I'll also consider the option you mention

a flag could set preserve_interfaces to true across all uses

I'll get back once I have decided on a design.

s-perron added a commit to s-perron/SPIRV-Tools that referenced this issue Jan 10, 2024
The optimizer is able to preserve the interface variables of the
shaders, but that feature has not been exposed to the command line
tool.

This commit adds an option `--preserve-interface` to spirv-opt that will
cause all calls to ADCE to leave the input and output variables, even if
the variable is unused. It will apply regardless of where the option
appears on the command line.

Fixes KhronosGroup#5522
@alecazam
Copy link
Author

alecazam commented Jan 10, 2024

I don't know what exactly the optimizers do, but they break most Android drivers. So we end up having to explicitly add/remove optimization stages. Also we need reflection at the same time as spirv codegen, but glslc can't seem to do that. So we end up running glslc, then spirv-opt with the following settings, then spirv-cross to get to MSL/HLSL. That's a lot of machinery that often breaks. We are looking forward to moving to HLSL + DXC instead. So an explicit --eliminate-dead-code-aggressive-preserve-interfaces stage is also fine for our use.

Example of some optimization stages used for Android

                .. "--ssa-rewrite " <- Problems
		.. "--ccp "
--		.. "--eliminate-local-single-block " <- Problems
--		.. "--eliminate-local-single-store " <- Problems
		.. "--eliminate-dead-code-aggressive "
		.. "--loop-unroll "
--		.. "--simplify-instructions " <- Problems
		.. "--scalar-replacement=0 "
--		.. "--redundancy-elimination " <- Problems
--		.. "--simplify-instructions " <- Problems

For transpiles, this is all that we can get away with.

local metalAndHlslOpt = ""
		.. "--eliminate-dead-code-aggressive " <- breaks HLSL codegen for PS4
		.. "--eliminate-dead-branches "
		.. "--eliminate-dead-code-aggressive " <- breaks HLSL codegen for PS4
		.. "--eliminate-dead-variables "

@s-perron
Copy link
Collaborator

but they break most Android drivers

I cannot say for sure what is happening in your case, but in general we have found these type of issues to be bugs in the driver. The drivers are tested on the SPIR-V or GLSL that is in the CTS and or is commonly used. The optimization may create different code patterns that are legal, but untest in the driver. I know this will not help you, but it means that there is no much I can do.

Do you know how old the drivers are that you are targeting? Is it still true for the latest drivers?

We are looking forward to moving to HLSL + DXC instead.

If you are writing HLSL that is definitely the recommended path. Are you using the HLSL compiler in glslangvalidator?

So an explicit --eliminate-dead-code-aggressive-preserve-interfaces stage is also fine for our use.

I have a pr that adds a standalone --preserve-interfaces. Let me know if that works for you.

@alecazam
Copy link
Author

I have a pr that adds a standalone --preserve-interfaces. Let me know if that works for you.

Comments on the thread, but thanks for that addition. I hope that can land in the main codebase. For now, I'm able to use the old spriv-opt for HLSL/PSSL codegen. So that should buy more time for you to get feedback on the change.

If you are writing HLSL that is definitely the recommended path. Are you using the HLSL compiler in glslangvalidator?

I have an open-source hlslparser that gens HLSL and MSL directly without transpiling (and preserves comments and code structure). But at work, we'd just write the HLSL and run DXC for reflect and spirv codgen. Then PS4 can just take the HLSL, Switch/Win/Android take spirv, and MSL is still a transpile using spirv-cross but code is ugly and unreadable compared to original sources.

s-perron added a commit that referenced this issue Jan 12, 2024
The optimizer is able to preserve the interface variables of the
shaders, but that feature has not been exposed to the command line
tool.

This commit adds an option `--preserve-interface` to spirv-opt that will
cause all calls to ADCE to leave the input and output variables, even if
the variable is unused. It will apply regardless of where the option
appears on the command line.

Fixes #5522
@alecazam
Copy link
Author

alecazam commented Jan 29, 2024

Something isn't right about the new spirv-opt.exe. We have a platform that does no semantic matching of VS output to PS inputs on transpiled HLSL. So they're just sequentially paired. So the semantics are correct but ignored by the compiler. When ADCE strips the inputs from the fragment shader then hilarity/chaos ensues. I built a custom build of spriv-opt, set --preserve-interface on just the fragment shaders, and get all white rendering. Leaving it out is also bad too. The transpile settings are above.

So our workaround for now will be to set KeepAlive to keep inputs from being stripped, but this is whack-a-mole especially when commenting out large swathes of code for hotloading/testing shaders, or for deeply variant-rich shaders which ifdef around inputs/outputs or code in the shader that then causes the inputs to be stripped. We can use the 12/24 spriv-opt.exe without --preserve-interface set then.

Alternatively, we also fallback to the old spriv-opt.exe, but that is less than ideal.

@alecazam
Copy link
Author

Seems like on this platform, we are responsible for the semantic pairing. So I'm connecting up that code. Then we should be okay with the new system of ADCE stripping inputs from the VS ouputs/PS inputs. We were already fixing up the VS inputs. It's just odd that the shader compiler doesn't default to this pairing, and ignores all the semantic definitions.

@s-perron
Copy link
Collaborator

It's just odd that the shader compiler doesn't default to this pairing, and ignores all the semantic definitions.

When generating SPIR-V, we assume you are targeting Vulkan. We optimize the SPIR-V according to the semantics of the Vulkan environment. Vulkan has no concept of the semantic definitions, so the compiler treats it only as debug information, which is conserved on a best effort basis.

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 a pull request may close this issue.

2 participants