Re: [Mesa-dev] [PATCH] glsl/ast: don't allow subroutine uniform comparisons
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
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
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
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
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
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
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
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