On 10/19/2016 02:17 PM, Brian Paul wrote: > On 10/19/2016 02:40 PM, Ian Romanick wrote: >> On 10/19/2016 11:11 AM, 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://lists.freedesktop.org/archives/mesa-dev/2016-October/132452.html >>> >>> 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. >> >> Why are we marking a uniform as invariant in the first place? That >> sounds boats. > > The shader author is marking the gl_Position VS output as invariant and > calling ftransform(): > > invariant gl_Position; > void main() > { > gl_Position = ftransform(); > } > > ftransform() expands into gl_ModelviewProjectionMatrix * gl_Vertex. > Then, afaict, the propagation pass marks gl_ModelviewProjectionMatrix as > invariant, but that disagrees with the original declaration of the > matrix and linking fails. > > That's my superficial understanding of it. > > Do you want me to hold off on pushing the patch? I'd really like to get > this or another fix in place.
I'm running this https://cgit.freedesktop.org/~idr/mesa/commit/?h=jenkins&id=e406683dcc993e55b38170aff698470b28909e2f through our CI right now. If there are no regressions and it fixes your problem, I think it's a better solution. If it doesn't meet those criteria, let's go ahead with Ken's patch. > -Brian > >>> 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 <kenn...@whitecape.org> >>> --- >>> 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 :) >>> >>> 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 >> mesa-dev@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev >> > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
