Turned on preserve bindings to impellerc#167203
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
|
@flar do you know, do the runtime shaders use impellerc too? I tested this with our own shaders. It has to, right? |
|
Everything uses impellerc |
|
(the difference is in vulkan vs runtime-stage vulkan) |
TIL - what Jonah said... |
|
The failures on CI seem apropos. It looks like we had 16bit float uniforms that were being removed with the dead code but now that they are present they are being flagged by validation layers. Example: |
|
I can't reproduce the failure locally. I see a different validation error even without this PR |
|
It's also possible that shaderc is transforming f16 to f32 without the preserve flag set. |
|
I'm unable to reproduce the failure locally when patching #167360 onto this PR. I'm not sure if it's related. |
36bf535 to
40b86b3
Compare
|
I'm rebasing against #167360 to see if we get the errors on CI still. |
|
I looked through the diff in shaderc that introduced this flag ( https://chromium.googlesource.com/external/github.com/google/shaderc/+/ca4c38cbc8137fba6fc3ddbf0d95362a04612fa2%5E%21/ ) and it really looks like this is a case where f16 uniforms were being stripped out because they weren't used, and now they are sticking around. The error messages aren't helpful about what variables those are though and I can't reproduce it locally since you need a device that doesn't support f16. |
40b86b3 to
3a09f08
Compare
|
|
||
| options.SetAutoBindUniforms(true); | ||
| options.SetAutoMapLocations(true); | ||
| options.SetPreserveBindings(true); |
There was a problem hiding this comment.
Try setting this to true only when this is one of the "runtime stage" modes (runtime-stage-vulkan, et cetera) those are the developer authored shaders
There was a problem hiding this comment.
I was hoping to get this working for us too since it makes working on the shaders a pain.
|
I'm able to reproduce this on my linux machine which is x86_64 (I had to download the validation layers myself): |
|
I tried to find the culprit by disassembling the spirv files and trying to find the 16bit uniform that is causing the error Didn't show up anything. These look potentially apropos: |
|
The culprit is a fragment shader with the name "half". I'm not sure where that is coming from: |
|
ah test fixture 🤔 |
|
Okay fixed it by not loading that shader if we don't support float16. I looked at the pr that introduced it and it doesn't look like we ever use it. We just compiled it. I think this points to some flaw in our thinking about float16 in vulkan. If this fragment shader was actually used we'd have to compile 2 different variants and load the appropriate one when the shader library is initialized. |
69cd243 to
b4426e8
Compare
fixes #116900
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.