Hi Tapani, Thanks for the review! I'll add the needed space.
On Fri, Sep 7, 2018 at 8:16 AM, Tapani Pälli <tapani.pa...@intel.com> wrote: > LGTM > > It would be nice to have this as part of 'cross_validate_globals' or some > other pass but considering how special/specific rules we are dealing with > here IMO it's fine to have a separate pass for it. > > Reviewed-by: Tapani Pälli <tapani.pa...@intel.com> > > One tiny nit below .. > > > On 08/29/2018 12:16 PM, Vadym Shovkoplias wrote: > >> From Section 4.6.4 (Invariance and Linkage) of the GLSL ES 1.0 >> specification >> >> "The invariance of varyings that are declared in both the vertex and >> fragment shaders must match. For the built-in special variables, >> gl_FragCoord can only be declared invariant if and only if >> gl_Position is declared invariant. Similarly gl_PointCoord can only >> be declared invariant if and only if gl_PointSize is declared >> invariant. It is an error to declare gl_FrontFacing as invariant. >> The invariance of gl_FrontFacing is the same as the invariance of >> gl_Position." >> >> Fixes: >> * glsl-pcoord-invariant.shader_test >> * glsl-fcoord-invariant.shader_test >> * glsl-fface-invariant.shader_test >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107734 >> Signed-off-by: Vadym Shovkoplias <vadym.shovkopl...@globallogic.com> >> --- >> src/compiler/glsl/linker.cpp | 66 ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 66 insertions(+) >> >> diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp >> index dbd76b7fcc..ca569bbea9 100644 >> --- a/src/compiler/glsl/linker.cpp >> +++ b/src/compiler/glsl/linker.cpp >> @@ -1286,6 +1286,66 @@ interstage_cross_validate_uniform_blocks(struct >> gl_shader_program *prog, >> return true; >> } >> +/** >> + * Verifies the invariance of built-in special variables. >> + */ >> +static bool >> +validate_invariant_builtins(struct gl_shader_program *prog, >> + const gl_linked_shader *vert, >> + const gl_linked_shader *frag) >> +{ >> + const ir_variable *var_vert; >> + const ir_variable *var_frag; >> + >> + if (!vert|| !frag) >> + return true; >> > > space before ||, > > I thought it was funny to have this check here since with GLES 1,2 you do > need both vert and frag to be present. But it seems we do that check only > later so I think this is fine. > > > > + >> + /* >> + * From OpenGL ES Shading Language 1.0 specification >> + * (4.6.4 Invariance and Linkage): >> + * "The invariance of varyings that are declared in both the >> vertex and >> + * fragment shaders must match. For the built-in special >> variables, >> + * gl_FragCoord can only be declared invariant if and only if >> + * gl_Position is declared invariant. Similarly gl_PointCoord can >> only >> + * be declared invariant if and only if gl_PointSize is declared >> + * invariant. It is an error to declare gl_FrontFacing as >> invariant. >> + * The invariance of gl_FrontFacing is the same as the invariance >> of >> + * gl_Position." >> + */ >> + var_frag = frag->symbols->get_variable("gl_FragCoord"); >> + if (var_frag && var_frag->data.invariant) { >> + var_vert = vert->symbols->get_variable("gl_Position"); >> + if (var_vert && !var_vert->data.invariant) { >> + linker_error(prog, >> + "fragment shader built-in `%s' has invariant qualifier, " >> + "but vertex shader built-in `%s' lacks invariant >> qualifier\n", >> + var_frag->name, var_vert->name); >> + return false; >> + } >> + } >> + >> + var_frag = frag->symbols->get_variable("gl_PointCoord"); >> + if (var_frag && var_frag->data.invariant) { >> + var_vert = vert->symbols->get_variable("gl_PointSize"); >> + if (var_vert && !var_vert->data.invariant) { >> + linker_error(prog, >> + "fragment shader built-in `%s' has invariant qualifier, " >> + "but vertex shader built-in `%s' lacks invariant >> qualifier\n", >> + var_frag->name, var_vert->name); >> + return false; >> + } >> + } >> + >> + var_frag = frag->symbols->get_variable("gl_FrontFacing"); >> + if (var_frag && var_frag->data.invariant) { >> + linker_error(prog, >> + "fragment shader built-in `%s' can not be declared as >> invariant\n", >> + var_frag->name); >> + return false; >> + } >> + >> + return true; >> +} >> /** >> * Populates a shaders symbol table with all global declarations >> @@ -5011,6 +5071,12 @@ link_shaders(struct gl_context *ctx, struct >> gl_shader_program *prog) >> lower_named_interface_blocks(mem_ctx, >> prog->_LinkedShaders[i]); >> } >> + if (prog->IsES && prog->data->Version == 100) >> + if (!validate_invariant_builtins(prog, >> + prog->_LinkedShaders[MESA_SHADER_VERTEX], >> + prog->_LinkedShaders[MESA_SHADER_FRAGMENT])) >> + goto done; >> + >> /* Implement the GLSL 1.30+ rule for discard vs infinite loops Do >> * it before optimization because we want most of the checks to get >> * dropped thanks to constant propagation. >> >> _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > -- Vadym Shovkoplias | Senior Software Engineer GlobalLogic P +380.57.766.7667 M +3.8050.931.7304 S vadym.shovkoplias www.globallogic.com <http://www.globallogic.com/> http://www.globallogic.com/email_disclaimer.txt
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
