[Mesa-dev] [PATCH] glsl/ast: don't allow subroutine uniform comparisons

[Andres Gomez]

Hi,

Just dropped:
https://lists.freedesktop.org/archives/mesa-dev/2016-July/123485.html

I didn't realize there was already this thread open.

On Tue, 2016-06-07 at 09:59 -0700, Ian Romanick wrote:
> On 06/06/2016 10:20 PM, Dave Airlie wrote:
> > From: Dave Airlie <airlied at redhat.com>
> > 
> > This fixes:
> > GL45-CTS.shader_subroutine.subroutines_cannot_be_assigned_float_int_values_or_be_compared
> > 
> > though I'm not 100% sure why this is illegal from the spec,
> > but it makes us pass the test, and I really can't see a use case for this.
> 
> I think the test is wrong.  Section 5.9 (Expressions) of the GLSL 4.5
> spec clearly says:
> 
>     The equality operators equal (==), and not equal (!=) operate on
>     all types (except aggregates that contain opaque types).

In my opinion, the specs are somehow contradictory or not completely
clear.

AFAIU, subroutine variables are to be used just in the way functions
are called. Although the spec doesn't say it explicitly, this means
that these variables are not to be used in any other way than those
left for function calls. Therefore, a comparison between 2 subroutine
variables should also cause a compilation error.

>From The OpenGL® Shading Language 4.40, page 117:

  "  To use subroutines, a subroutine type is declared, one or more
     functions are associated with that subroutine type, and a
     subroutine variable of that type is declared. The function
     currently assigned to the variable function is then called by
     using function calling syntax replacing a function name with the
     name of the subroutine variable. Subroutine variables are
     uniforms, and are assigned to specific functions only through
     commands (UniformSubroutinesuiv) in the OpenGL API."

>From The OpenGL® Shading Language 4.40, page 118:

  "  Subroutine uniform variables are called the same way functions
     are called. When a subroutine variable (or an element of a
     subroutine variable array) is associated with a particular
     function, all function calls through that variable will call that
     particular function."

> As much as anyone would use subroutines, you could imagine this being
> used like:
> 
>     value = foo(param1, param2);
>     if (foo != bar)
>         value += bar(param1, param2);

If that would be the case, and we agree that subroutines can be
compared, then we have, at least, some other bug to correct.

I've made some piglit tests with the following scenarios:
 * == comparison result:
    * foo and bar point to the same subroutine function -> false
    * foo and bar point to different subroutine functions -> false
 * != comparison result:
    * foo and bar point to the same subroutine function -> false
    * foo and bar point to different subroutine functions -> false

So, what would be the conclusion? Do we allow subroutine variables comparison?

FTR, I passed this patch through an "all" piglit run and through GL44 CTS and it doesn't cause any regression.

> 
> > Signged-off-by: Dave Airlie <airlied at redhat.com>
> > ---
> >  src/compiler/glsl/ast_to_hir.cpp | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/src/compiler/glsl/ast_to_hir.cpp b/src/compiler/glsl/ast_to_hir.cpp
> > index b7192b2..fbd3256 100644
> > --- a/src/compiler/glsl/ast_to_hir.cpp
> > +++ b/src/compiler/glsl/ast_to_hir.cpp
> > @@ -1484,6 +1484,12 @@ ast_expression::do_hir(exec_list *instructions,
> >                           "operand of type 'void' or a right operand of type "
> >                           "'void'", (this->oper == ast_equal) ? "==" : "!=");
> >           error_emitted = true;
> > +      } else if (op[0]->type->is_subroutine() || op[1]->type->is_subroutine()) {
> > +         _mesa_glsl_error(& loc, state, "`%s':  wrong operand types: "
> > +                         "no operation `%1$s' exists that takes a left-hand "
> > +                         "operand of type 'subroutine' or a right operand of type "
> > +                         "'subroutine'", (this->oper == ast_equal) ? "==" : "!=");
> > +         error_emitted = true;
> >        } else if ((!apply_implicit_conversion(op[0]->type, op[1], state)
> >             && !apply_implicit_conversion(op[1]->type, op[0], state))
> >            || (op[0]->type != op[1]->type)) {
> > 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-- 

Tanty

Sent from my Igalia pOmeron