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

Opt out of "Merge for-declaration with the previous declaration" #397

Closed
thscott opened this issue May 14, 2024 · 4 comments · Fixed by #407
Closed

Opt out of "Merge for-declaration with the previous declaration" #397

thscott opened this issue May 14, 2024 · 4 comments · Fixed by #407

Comments

@thscott
Copy link

thscott commented May 14, 2024

Would it be possible to have a way to opt out of the change introduced in #371?

The form int a=2, b=3; for (; ...) is not valid in GLSL ES, and hence not valid in WebGL 1.

See Appendix A of the spec here, specifically these lines:

  • The for statement has the form:
    • for ( init-declaration ; condition ; expression ) statement
  • init-declaration has the form:
    • type-specifier identifier = constant-expression

Consequently the loop variable cannot be a global variable.

@laurentlb
Copy link
Owner

laurentlb commented May 14, 2024

Nice finding, thanks!
I should just rollback this commit. The optim is not worth it, and there was another related bug report (#375).

@therontarigo
Copy link
Contributor

therontarigo commented May 15, 2024

I'd rather not lose this optimization because of such an ancient GLES version.
Checking the compression stats, the only shaders for which it hurt compression were those also using --no-renaming - which breaks the context models anyway, so they shouldn't be trusted too much.

add --language option with hlsl, glsl, glsl100es glsl300es as choices
since we already have --hlsl option for language selection, only a new --version option with 460 100es 300es as choices would be needed.
With version selection we don't need an optimization-specific flag here, even if this is the only thing it will affect thus far. Webgl1 (100es) is the only lang with this restriction. Desktop GLSL also allows int literals in more places than 300es does, though this isn't exploited yet.

therontarigo added a commit to therontarigo/Shader_Minifier that referenced this issue May 15, 2024
therontarigo added a commit to therontarigo/Shader_Minifier that referenced this issue May 15, 2024
therontarigo added a commit to therontarigo/Shader_Minifier that referenced this issue May 15, 2024
therontarigo added a commit to therontarigo/Shader_Minifier that referenced this issue May 15, 2024
@laurentlb
Copy link
Owner

In 5b8f945#diff-5a7085bc7afa14a48d0b7c3c68992949b5c04e13ef2f76a46eb17d106b6c6d62, the compression tests are all done with the default flags and include renaming (so they don't match the .expected files). Based on that, this optimization doesn't save space after compression.

A reason to keep it is that it can be useful for people who care about the uncompressed size. It may also help future optimizations (maybe).

@therontarigo
Copy link
Contributor

Thanks, clearly I was thinking compression inputs differed only by --format text. I don't mind seeing it disabled for now. This puts #398 as low priority, though it relates also to another optimization I'm currently working.

therontarigo added a commit to therontarigo/Shader_Minifier that referenced this issue May 16, 2024
laurentlb added a commit that referenced this issue May 24, 2024
laurentlb added a commit that referenced this issue May 24, 2024
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.

3 participants