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.

Sign up for GitHub

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

Jump to bottom

Issue with EliminateDeadOutputStores when reading from out variable #5516

Open
mbechard opened this issue Jan 2, 2024 · 7 comments · May be fixed by #5672
Open

Issue with EliminateDeadOutputStores when reading from out variable #5516

mbechard opened this issue Jan 2, 2024 · 7 comments · May be fixed by #5672
Assignees
@jeremy-lunarg
Labels
component:optimization

Comments

@mbechard
Copy link

mbechard commented Jan 2, 2024

Hey,
If I have a GLSL shader that is reading from an 'out' variable such as

out vec4 pos;
void main()
{
    pos = vec4(0.0);
    gl_Position = pos;
}

And then I run the resulting SPIRV through the EliminateDeadOutputStores() pass, I get an assert saying 'unexpected use of output variable'.
This should be legal code since "During shader execution they will behave as normal unqualified global variables" as per the GLSL spec.
@jeremy-lunarg can you take a look since you took over maintenance of this feature?
Thanks!
Malcolm
@mbechard
Copy link
Author

Sorry to ping this, it's affecting a lot of my users due to this being a common writing workflow in shaders it seems.

@mbechard
Copy link
Author

mbechard commented Apr 1, 2024

With the recent paper highlighting this workflow, I'd again like to ping this one

@greg-lunarg
Copy link
Contributor

greg-lunarg commented Apr 1, 2024 via email

@jeremy-lunarg
Copy link
Contributor

@mbechard Sorry. This fell through the cracks. I'll start looking at this tomorrow.

@jeremy-lunarg
Copy link
Contributor

This looks like it's going to be non-trivial. I think pos should be eliminated but currently store-load elimination does not operate on output variables and the shader would be broken if we eliminated pos. We can do store-load elimination on pos but it would require writing a new pass. At the very least we shouldn't assert and we should silently ignore pos during EliminateDeadOutputStores. I think I'll work on that first.

@mbechard
Copy link
Author

mbechard commented Apr 2, 2024 •
edited
Loading

Thanks. right so more specifically this occurs if 'pos' is not consumed by the pixel shader, and thus it tries to be eliminated from the vertex shader during the call to SpirvToolsEliminateDeadOutputStores(), which causes this issue.

Ideally if the pass is trying to eliminate an output variable that is being used for a store-load, it should be converted to a temporary variable. Leaving it as-is would work for standalone outputs, but would cause an interface mismatch if the output is part of a block, where it's been eliminated from the block in the pixel but getting left as-is in the vertex

jeremy-lunarg added a commit to jeremy-lunarg/SPIRV-Tools that referenced this issue May 16, 2024
@jeremy-lunarg
spirv-opt: Demote r/w output variables to private
55b6cb8 
Fix KhronosGroup#5516

Pass: eliminate dead output stores

Stores to write-only output variables are safe to eliminate; however,
stores to read-write output variables may be required for functional
correctness. If a read-write output variable is detected, then it will
be demoted to a private variable and any associated stores will not be
eliminated.
@jeremy-lunarg jeremy-lunarg linked a pull request May 16, 2024 that will close this issue
Open
@mbechard
Copy link
Author

mbechard commented Dec 4, 2024

Hey, wanted to ping this one again. It's a pretty commonly used workflow in GLSL, and a large problem that it's failing here. Any chance this can be finished off?

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

@jeremy-lunarg jeremy-lunarg

Labels
component:optimization
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

WIP: spirv-opt: Demote r/w output variables to private jeremy-lunarg/SPIRV-Tools
5 participants
@mbechard @jeremy-lunarg @greg-lunarg @s-perron and others