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

[Ian Romanick]

On 07/19/2016 01:45 PM, Ian Romanick wrote:
> On 07/19/2016 06:54 AM, Andres Gomez wrote:
>> 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 

 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?
> 
> There is no conclusion yet.  I opened a Khronos gitlab tracker (right
> after Dave sent his original patch) for the CTS.  I'll try to get it on
> the conference call agenda for this week.

It is decided... the test will stand as-is, and the GLSL spec will be
updated to explicitly say that subroutine uniforms cannot be compared
using == or !=.

So... I think I like Andres's patch slightly better than Dave's, but I
don't care too much.  Either patch can have my

Reviewed-by: Ian Romanick 

>> FTR, I passed this patch through an "all" piglit run and through GL44 CTS 
>> and it doesn't cause any regression.
> 
> ___
> 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

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

[Andres Gomez]

On Tue, 2016-07-19 at 13:45 -0700, Ian Romanick wrote:
> On 07/19/2016 06:54 AM, Andres Gomez wrote:
...
> > So, what would be the conclusion? Do we allow subroutine variables 
> > comparison?
> 
> There is no conclusion yet.  I opened a Khronos gitlab tracker (right
> after Dave sent his original patch) for the CTS.  I'll try to get it on
> the conference call agenda for this week.

Thanks, Ian! ☺

-- 

Br,

Andres
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Ian Romanick]

On 07/19/2016 06:54 AM, Andres Gomez wrote:
> 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 
>>>
>>> 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?

There is no conclusion yet.  I opened a Khronos gitlab tracker (right
after Dave sent his original patch) for the CTS.  I'll try to get it on
the conference call agenda for this week.

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

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Andres Gomez]

On Tue, 2016-06-07 at 15:20 +1000, Dave Airlie wrote:
> From: Dave Airlie 
> 
> 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.
> 
> Signged-off-by: Dave Airlie 
> ---
>  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()) {

Shouldn't we use type->contains_subroutine() ?

That way we would be having into account subroutine arrays too.

Other than the open discussion about whether this is allowed by the
specs or not, this is:

Reviewed-by: Andres Gomez 
-- 

Br,

Andres
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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 
> > 
> > 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 
> > ---
> >  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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-- 

Tanty

Sent from my Igalia pOmeron
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Ian Romanick]

On 06/06/2016 10:20 PM, Dave Airlie wrote:
> From: Dave Airlie 
> 
> 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).

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);

> Signged-off-by: Dave Airlie 
> ---
>  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Alejandro Piñeiro]

On 07/06/16 07:20, Dave Airlie wrote:
> From: Dave Airlie 
>
> 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,

Reading a little the spec, I also didn't find any explicit reference on
the spec. Having said so ...
> but it makes us pass the test, and I really can't see a use case for this.

... yes, I agree with this.

>
> Signged-off-by: Dave Airlie 

typo here: Signed-off-by

> ---
>  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)) {

Reviewed-by: Alejandro Piñeiro 

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Dave Airlie]

From: Dave Airlie 

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.

Signged-off-by: Dave Airlie 
---
 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)) {
-- 
2.5.5

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev