On 10/19/2016 12:11 PM, Kenneth Graunke wrote:
Brian found a bug with my "inline built-ins immediately" code for shaders
which use ftransform() and declare gl_Position invariant:
https://urldefense.proofpoint.com/v2/url?u=https-3A__lists.freedesktop.org_archives_mesa-2Ddev_2016-2DOctober_132452.html&d=CwIBAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=T0t4QG7chq2ZwJo6wilkFznRSFy-8uDKartPGbomVj8&m=FUsV9E5siNJA_21T5neVxOfd-A-t334aLcd7uj41cy0&s=WGbph-T-_o7nJ30qH0kn674oIEsEM_CRXPSGA0yHNrg&e=
Before my patch, things worked due to a specific order of operations:
1. link_intrastage_varyings imported the ftransform function into the VS
2. cross_validate_uniforms() ran and signed off that everything matched
3. do_common_optimization did both inlining and invariance propagation,
making the VS/FS versions of gl_ModelViewProjectionMatrix have
different invariant qualifiers...but after the check in step 2,
so we never raised an error.
After my patch, ftransform() is inlined right away, and at compile time,
do_common_optimization propagates the invariant qualifier to the
gl_ModelViewProjectionMatrix. When the linker eventually happens, it
detects the mismatch.
I can't see any good reason to raise a linker error based on qualifiers
we internally applied to built-in variables - it's not the application's
fault. It's either not a problem, or it's our fault.o
We should probably rework invariance, but this should keep us limping
along for now. It's definitely a hack.
Signed-off-by: Kenneth Graunke <[email protected]>
---
src/compiler/glsl/linker.cpp | 33 +++++++++++++++++++++------------
1 file changed, 21 insertions(+), 12 deletions(-)
Hi Brian,
I'm on vacation today through Friday, so I likely won't be able to
push this until next week. If people are okay with my hack, feel free
to push it before I get back :)
OK. Tested and works for me. I'd also like to tag this for the 13.0
branch. If there's no other discussion, I'll push this later.
Cc: "13.0" <[email protected]>
Reviewed-by: Brian Paul <[email protected]>
Tested-by: Brian Paul <[email protected]>
Thanks, Ken!
diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp
index 8599590..66f9e76 100644
--- a/src/compiler/glsl/linker.cpp
+++ b/src/compiler/glsl/linker.cpp
@@ -1038,12 +1038,28 @@ cross_validate_globals(struct gl_shader_program *prog,
}
}
- if (existing->data.invariant != var->data.invariant) {
- linker_error(prog, "declarations for %s `%s' have "
- "mismatching invariant qualifiers\n",
- mode_string(var), var->name);
- return;
+ /* Skip invariant/precise checks for built-in uniforms.
+ * If they're used in an invariant calculation, the invariance
+ * propagation pass might mark these. But that's not an error
+ * on the programmer's part - it's our problem. It shouldn't
+ * actually matter anyway, so ignore it.
+ */
+ if (var->get_num_state_slots() == 0) {
+ if (existing->data.invariant != var->data.invariant) {
+ linker_error(prog, "declarations for %s `%s' have "
+ "mismatching invariant qualifiers\n",
+ mode_string(var), var->name);
+ return;
+ }
+
+ if (prog->IsES && existing->data.precision != var->data.precision)
{
+ linker_error(prog, "declarations for %s `%s` have "
+ "mismatching precision qualifiers\n",
+ mode_string(var), var->name);
+ return;
+ }
}
+
if (existing->data.centroid != var->data.centroid) {
linker_error(prog, "declarations for %s `%s' have "
"mismatching centroid qualifiers\n",
@@ -1062,13 +1078,6 @@ cross_validate_globals(struct gl_shader_program *prog,
mode_string(var), var->name);
return;
}
-
- if (prog->IsES && existing->data.precision != var->data.precision) {
- linker_error(prog, "declarations for %s `%s` have "
- "mismatching precision qualifiers\n",
- mode_string(var), var->name);
- return;
- }
} else
variables->add_variable(var);
}
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
