spirv-opt: CreateRemoveUnusedInterfaceVariablesPass should preserve interface if asked to#6674
spirv-opt: CreateRemoveUnusedInterfaceVariablesPass should preserve interface if asked to#6674uwx wants to merge 1 commit into
Conversation
…nterface if asked to
uwx
changed the title
|
|
|
Oh, I just noticed clang-format touched a bunch of code that I didn't edit. Let me know if it needs to be undone. |
|
|
||
| RemoveUnusedInterfaceVariablesPass::Status | ||
| RemoveUnusedInterfaceVariablesPass::Process() { | ||
| if (preserve_interface_) return Status::SuccessWithoutChange; |
There was a problem hiding this comment.
I don't think this is the correct solution. We need to consider all of the uses cases.
This pass was intended to cover the cases where there were multiple entry points. If a variables is used one entry point, and not in another, we do not want it on the OpEntryPoint of the one in which it is not used. Based on the comment from the PR that added the pass, the issue was that some variables are added to the OpEntryPoint for simplicity by front-ends that do not know which ones to add (See microsoft/DirectXShaderCompiler#4621).
The other issue is that the Preserve interface flag does stops the removal of the input and output variables. However, we should still remove the resource variables, which is covered by another flag.
Can you provide more details on exactly what you want out of this?
@dnovillo is also interested in this flag, getting the use cases right might help us get something that works in all cases.
There was a problem hiding this comment.
Basically our issue in SDL_gpu is that we need the byte for byte structure of the vertex attributes to be the same as what's declared in the shader otherwise we're uploading incorrect vertex data to the GPU. So we need to keep unused variables. cc @thatcosmonaut
There was a problem hiding this comment.
That's right, we're experiencing gotchas because unused variables are optimized out and then pipeline vertex definitions don't match the compiled shader.
There was a problem hiding this comment.
Would it works if the option applied to inputs and outputs that are not builtin? That is, we still remove builtin inputs. The other problem we need to fix is that some builtins get added to shader stages where they are not allowed. We have to remove them.
4 participants
This fixes #6673: CreateRemoveUnusedInterfaceVariablesPass was not respecting the
preserve_interfaceflag passed in and collapsing unused attributes. The simple test case in microsoft/DirectXShaderCompiler#7982 now preserves both TEXCOORD0 and TEXCOORD1 with this patch applied:I chose to pass the argument in instead of not registering the pass because that's what's done for the other passes, even though for this one it makes it a total no-op. Let me know if that is inadequate.