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 disable_for_loops to CompilerGLSL options #490

Closed
wants to merge 1 commit into from
Closed

Add disable_for_loops to CompilerGLSL options #490

wants to merge 1 commit into from

Conversation

zeux
Copy link
Contributor

@zeux zeux commented Mar 6, 2018

Optimization passes in SPIRV-Tools can merge the entire loop body block
into the continue block. This results in shaders that are very hard to read;
this flag works around the problem.

Optimization passes in SPIRV-Tools can merge the entire loop body block
into the continue block. This results in shaders that are very hard to read;
this flag works around the problem.
@zeux
Copy link
Contributor Author

zeux commented Mar 6, 2018

Actually, I might have rushed it a bit. I think on one of our shaders disable_for_loops=true results in an infinite loop, one of the break statements should've been continue... It appears that the generic loop emission path isn't handling this well.

@zeux
Copy link
Contributor Author

zeux commented Mar 6, 2018

... I see. In that shader, the branch that branches out of the loop seems to jump to a block that is simultaneously classified as break and continue (with is_break & is_continue); I think it's a break block for the inner loop and a continue loop for the outer loop. Changing the order of is_break & is_continue checks in CompilerGLSL::branch fixes this but I'm sure this is not the correct solution.

Is there an expectation that the generic loop emission path should handle arbitrary loops? If yes I can open a separate issue with the problematic shader.

@HansKristian-Work
Copy link
Contributor

HansKristian-Work commented Mar 6, 2018

This is a bit inelegant, I think a better solution will be to detect this kind of loop where loop header jumps straight into continue block, and add an option to special case those loops. It is not possible to always enable that path due to an obscure ESSL 1.0 rule.

I can think something like adding a new loop type like MergeToContinueForLoop. This would be skipped for ESSL 1.0, but allowed for modern targets. The loop emitter can just emit the continue block in the loop body. There might be some complications with branching to loop header, and creating new variables inside the loop body, but I think those should be handled already ...

There is a fallback "ComplexLoop", which is basically for (;;) { ... }, which handles all cases, but it's known to segfault certain shader compilers ... :')

@zeux
Copy link
Contributor Author

zeux commented Mar 7, 2018

@HansKristian-ARM Yeah so I think there are two issues here:

  1. With SPIRV-Tools advances, not using complex loop fallback means a lot of loops look very awkward
  2. Complex loop fallback appears to malfunction on at least one of our shaders

2 is a bug, 1 is an inconvenience :) FWIW we only care about this for Metal right now where we occasionally have to source-level-debug SPIRV-Cross output.

@HansKristian-Work
Copy link
Contributor

HansKristian-Work commented Mar 7, 2018

Uh ... right, no. 2 would be a bug. You have a sample SPV that triggers this?
Do you mean that enabling complex loop fallback for these "smash-everything-into-continue-block" shaders triggers the issue in ComplexLoopFallback?

I don't think complex loop fallback has been tested with this weird loop scheme (because those loops would never trigger complex loops), so it's not unlikely there's a bug here.

@zeux
Copy link
Contributor Author

zeux commented Mar 7, 2018

Do you mean that enabling complex loop fallback for these "smash-everything-into-continue-block" shaders triggers the issue in ComplexLoopFallback?

Yeah - correct. complex loop fallback turns one of the loops into an infinite loop. I'll post a separate bug report with this. I agree that otherwise it would make more sense to automatically detect these crazy loops instead of the user having to manually specify this as an option.

@HansKristian-Work
Copy link
Contributor

HansKristian-Work commented Mar 7, 2018

Ok, I think I have a good understanding what's going on, I'll repro this on my end and see what's going on ... I'll try to come up with a "clean" solution for this that does not require an option.

@zeux
Copy link
Contributor Author

zeux commented Mar 8, 2018

That makes sense, thanks!

@zeux zeux closed this Mar 8, 2018
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.

2 participants