Re: [Mesa-dev] [PATCH 08/10] glsl: fix new gcc6 warnings

[Emil Velikov]

On 17 February 2016 at 22:43, Rob Clark  wrote:
> On Wed, Feb 17, 2016 at 4:40 PM, Patrick Baggett
>  wrote:
>> On Wed, Feb 17, 2016 at 3:35 PM, Rob Clark  wrote:
>>> src/compiler/glsl/lower_discard_flow.cpp:79:1: warning: ‘ir_visitor_status 
>>> {anonymous}::lower_discard_flow_visitor::visit_enter(ir_loop_jump*)’ 
>>> defined but not used [-Wunused-function]
>>>  lower_discard_flow_visitor::visit_enter(ir_loop_jump *ir)
>>>  ^~
>>>
>>> The base class method that was intended to be overridden was
>>> 'visit(ir_loop_jump *ir)', not visit_entire().
>>>
>> Has there been a discussion about using the "override" keyword
>> (C++11)? It sounds like it could catch bugs like this, and if hidden
>> behind a #define, act as a no-op when C++11 is not supported. Although
>> obviously the new gcc6 warning is effectively doing much the same
>> thing...
>>
>
> tbh, I'm not sure what, if any, C++11 discussion has happened.. most
> of the code I deal with is C
>
Recently C++11 came in the context of nouveau's use of tr1 headers,
which are missing when building on *BSD (due to clang/libc++) or
android (in cases).
Note: clover and some parts of gallium/aux have been using c++11 for a while.

Neither of which applies here imho ?

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

Re: [Mesa-dev] [PATCH 08/10] glsl: fix new gcc6 warnings

[Rob Clark]

Thanks for running that thru CI..

if you have a chance, I wouldn't mind a review on v2 of 01/10 (since
that one is by far the most noisy gcc6 warning, and would be nice to
get that clean-up in before the branch point)

BR,
-R


On Wed, Feb 17, 2016 at 8:43 PM, Ian Romanick  wrote:
> I ran this through our CI, and I didn't see any regressions cause by it.
>  I think this is correct, so this patch is
>
> Reviewed-by: Ian Romanick 
>
> On 02/17/2016 01:35 PM, Rob Clark wrote:
>> src/compiler/glsl/lower_discard_flow.cpp:79:1: warning: ‘ir_visitor_status 
>> {anonymous}::lower_discard_flow_visitor::visit_enter(ir_loop_jump*)’ defined 
>> but not used [-Wunused-function]
>>  lower_discard_flow_visitor::visit_enter(ir_loop_jump *ir)
>>  ^~
>>
>> The base class method that was intended to be overridden was
>> 'visit(ir_loop_jump *ir)', not visit_entire().
>>
>> Signed-off-by: Rob Clark 
>> ---
>>  src/compiler/glsl/lower_discard_flow.cpp | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/compiler/glsl/lower_discard_flow.cpp 
>> b/src/compiler/glsl/lower_discard_flow.cpp
>> index 9d0a56b..9e3a7c0 100644
>> --- a/src/compiler/glsl/lower_discard_flow.cpp
>> +++ b/src/compiler/glsl/lower_discard_flow.cpp
>> @@ -62,8 +62,8 @@ public:
>> {
>> }
>>
>> +   ir_visitor_status visit(ir_loop_jump *ir);
>> ir_visitor_status visit_enter(ir_discard *ir);
>> -   ir_visitor_status visit_enter(ir_loop_jump *ir);
>> ir_visitor_status visit_enter(ir_loop *ir);
>> ir_visitor_status visit_enter(ir_function_signature *ir);
>>
>> @@ -76,7 +76,7 @@ public:
>>  } /* anonymous namespace */
>>
>>  ir_visitor_status
>> -lower_discard_flow_visitor::visit_enter(ir_loop_jump *ir)
>> +lower_discard_flow_visitor::visit(ir_loop_jump *ir)
>>  {
>> if (ir->mode != ir_loop_jump::jump_continue)
>>return visit_continue;
>>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 08/10] glsl: fix new gcc6 warnings

[Ian Romanick]

I ran this through our CI, and I didn't see any regressions cause by it.
 I think this is correct, so this patch is

Reviewed-by: Ian Romanick 

On 02/17/2016 01:35 PM, Rob Clark wrote:
> src/compiler/glsl/lower_discard_flow.cpp:79:1: warning: ‘ir_visitor_status 
> {anonymous}::lower_discard_flow_visitor::visit_enter(ir_loop_jump*)’ defined 
> but not used [-Wunused-function]
>  lower_discard_flow_visitor::visit_enter(ir_loop_jump *ir)
>  ^~
> 
> The base class method that was intended to be overridden was
> 'visit(ir_loop_jump *ir)', not visit_entire().
> 
> Signed-off-by: Rob Clark 
> ---
>  src/compiler/glsl/lower_discard_flow.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/compiler/glsl/lower_discard_flow.cpp 
> b/src/compiler/glsl/lower_discard_flow.cpp
> index 9d0a56b..9e3a7c0 100644
> --- a/src/compiler/glsl/lower_discard_flow.cpp
> +++ b/src/compiler/glsl/lower_discard_flow.cpp
> @@ -62,8 +62,8 @@ public:
> {
> }
>  
> +   ir_visitor_status visit(ir_loop_jump *ir);
> ir_visitor_status visit_enter(ir_discard *ir);
> -   ir_visitor_status visit_enter(ir_loop_jump *ir);
> ir_visitor_status visit_enter(ir_loop *ir);
> ir_visitor_status visit_enter(ir_function_signature *ir);
>  
> @@ -76,7 +76,7 @@ public:
>  } /* anonymous namespace */
>  
>  ir_visitor_status
> -lower_discard_flow_visitor::visit_enter(ir_loop_jump *ir)
> +lower_discard_flow_visitor::visit(ir_loop_jump *ir)
>  {
> if (ir->mode != ir_loop_jump::jump_continue)
>return visit_continue;
> 

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

Re: [Mesa-dev] [PATCH 08/10] glsl: fix new gcc6 warnings

[Rob Clark]

On Wed, Feb 17, 2016 at 4:40 PM, Patrick Baggett
 wrote:
> On Wed, Feb 17, 2016 at 3:35 PM, Rob Clark  wrote:
>> src/compiler/glsl/lower_discard_flow.cpp:79:1: warning: ‘ir_visitor_status 
>> {anonymous}::lower_discard_flow_visitor::visit_enter(ir_loop_jump*)’ defined 
>> but not used [-Wunused-function]
>>  lower_discard_flow_visitor::visit_enter(ir_loop_jump *ir)
>>  ^~
>>
>> The base class method that was intended to be overridden was
>> 'visit(ir_loop_jump *ir)', not visit_entire().
>>
> Has there been a discussion about using the "override" keyword
> (C++11)? It sounds like it could catch bugs like this, and if hidden
> behind a #define, act as a no-op when C++11 is not supported. Although
> obviously the new gcc6 warning is effectively doing much the same
> thing...
>

tbh, I'm not sure what, if any, C++11 discussion has happened.. most
of the code I deal with is C

BR,
-R

>
>> Signed-off-by: Rob Clark 
>> ---
>>  src/compiler/glsl/lower_discard_flow.cpp | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/compiler/glsl/lower_discard_flow.cpp 
>> b/src/compiler/glsl/lower_discard_flow.cpp
>> index 9d0a56b..9e3a7c0 100644
>> --- a/src/compiler/glsl/lower_discard_flow.cpp
>> +++ b/src/compiler/glsl/lower_discard_flow.cpp
>> @@ -62,8 +62,8 @@ public:
>> {
>> }
>>
>> +   ir_visitor_status visit(ir_loop_jump *ir);
>> ir_visitor_status visit_enter(ir_discard *ir);
>> -   ir_visitor_status visit_enter(ir_loop_jump *ir);
>> ir_visitor_status visit_enter(ir_loop *ir);
>> ir_visitor_status visit_enter(ir_function_signature *ir);
>>
>> @@ -76,7 +76,7 @@ public:
>>  } /* anonymous namespace */
>>
>>  ir_visitor_status
>> -lower_discard_flow_visitor::visit_enter(ir_loop_jump *ir)
>> +lower_discard_flow_visitor::visit(ir_loop_jump *ir)
>>  {
>> if (ir->mode != ir_loop_jump::jump_continue)
>>return visit_continue;
>> --
>> 2.5.0
>>
>> ___
>> 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 08/10] glsl: fix new gcc6 warnings

[Patrick Baggett]

On Wed, Feb 17, 2016 at 3:35 PM, Rob Clark  wrote:
> src/compiler/glsl/lower_discard_flow.cpp:79:1: warning: ‘ir_visitor_status 
> {anonymous}::lower_discard_flow_visitor::visit_enter(ir_loop_jump*)’ defined 
> but not used [-Wunused-function]
>  lower_discard_flow_visitor::visit_enter(ir_loop_jump *ir)
>  ^~
>
> The base class method that was intended to be overridden was
> 'visit(ir_loop_jump *ir)', not visit_entire().
>
Has there been a discussion about using the "override" keyword
(C++11)? It sounds like it could catch bugs like this, and if hidden
behind a #define, act as a no-op when C++11 is not supported. Although
obviously the new gcc6 warning is effectively doing much the same
thing...


> Signed-off-by: Rob Clark 
> ---
>  src/compiler/glsl/lower_discard_flow.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/compiler/glsl/lower_discard_flow.cpp 
> b/src/compiler/glsl/lower_discard_flow.cpp
> index 9d0a56b..9e3a7c0 100644
> --- a/src/compiler/glsl/lower_discard_flow.cpp
> +++ b/src/compiler/glsl/lower_discard_flow.cpp
> @@ -62,8 +62,8 @@ public:
> {
> }
>
> +   ir_visitor_status visit(ir_loop_jump *ir);
> ir_visitor_status visit_enter(ir_discard *ir);
> -   ir_visitor_status visit_enter(ir_loop_jump *ir);
> ir_visitor_status visit_enter(ir_loop *ir);
> ir_visitor_status visit_enter(ir_function_signature *ir);
>
> @@ -76,7 +76,7 @@ public:
>  } /* anonymous namespace */
>
>  ir_visitor_status
> -lower_discard_flow_visitor::visit_enter(ir_loop_jump *ir)
> +lower_discard_flow_visitor::visit(ir_loop_jump *ir)
>  {
> if (ir->mode != ir_loop_jump::jump_continue)
>return visit_continue;
> --
> 2.5.0
>
> ___
> 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 08/10] glsl: fix new gcc6 warnings

[Rob Clark]

On Tue, Feb 16, 2016 at 2:32 PM, Ian Romanick  wrote:
> On 02/16/2016 10:58 AM, Rob Clark wrote:
>> src/compiler/glsl/lower_discard_flow.cpp:79:1: warning: ‘ir_visitor_status 
>> {anonymous}::lower_discard_flow_visitor::visit_enter(ir_loop_jump*)’ defined 
>> but not used [-Wunused-function]
>>  lower_discard_flow_visitor::visit_enter(ir_loop_jump *ir)
>>  ^~
>>
>> Note, not sure if this was a latent bug?  Could be that was intended to
>> override visit(ir_loop_jump *)?
>
> I'll wager that is correct, and the bug has existed since day 1. :(
>
> To hit the bug, you'd need a loop with both a discard and a continue.  I
> suspect that is a rare combination.  To observe that the bug had been
> hit, you'd have to use a derivative (either via dFdx() and friends or
> texture()) in a particular way.  I'll have to try to think of a test case.

so, I ended up just hacking in a _mesa_print_ir() before/after
lower_discard_flow()..  not really a re-usable test, but enough for me
to verify what was going on.

> It might be easier to just add a unit test.  We know that
>
>for (int i = 0; i < x; i++) {
>   if (z)
>  continue;
>
>   if (y)
>  discard;
>}
>
> should get transformed to
>
>for (int i = 0; i < x; i++) {
>   if (z) {
>  if (discarded)
> break;

current state is that we miss this 'if (discarded) break;'  Renaming
that fxn to 'visit' instead of 'visit_enter' so it actually overrides
something fixes that.

BR,
-R


>  continue;
>   }
>
>   if (y) {
>  discarded = true;
>  discard;
>   }
>
>   if (discarded)
>  break;
>}
>
>> Signed-off-by: Rob Clark 
>> ---
>>  src/compiler/glsl/lower_discard_flow.cpp | 12 
>>  1 file changed, 12 deletions(-)
>>
>> diff --git a/src/compiler/glsl/lower_discard_flow.cpp 
>> b/src/compiler/glsl/lower_discard_flow.cpp
>> index 9d0a56b..bdb96b4 100644
>> --- a/src/compiler/glsl/lower_discard_flow.cpp
>> +++ b/src/compiler/glsl/lower_discard_flow.cpp
>> @@ -63,7 +63,6 @@ public:
>> }
>>
>> ir_visitor_status visit_enter(ir_discard *ir);
>> -   ir_visitor_status visit_enter(ir_loop_jump *ir);
>> ir_visitor_status visit_enter(ir_loop *ir);
>> ir_visitor_status visit_enter(ir_function_signature *ir);
>>
>> @@ -76,17 +75,6 @@ public:
>>  } /* anonymous namespace */
>>
>>  ir_visitor_status
>> -lower_discard_flow_visitor::visit_enter(ir_loop_jump *ir)
>> -{
>> -   if (ir->mode != ir_loop_jump::jump_continue)
>> -  return visit_continue;
>> -
>> -   ir->insert_before(generate_discard_break());
>> -
>> -   return visit_continue;
>> -}
>> -
>> -ir_visitor_status
>>  lower_discard_flow_visitor::visit_enter(ir_discard *ir)
>>  {
>> ir_dereference *lhs = new(mem_ctx) ir_dereference_variable(discarded);
>>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 08/10] glsl: fix new gcc6 warnings

[Rob Clark]

On Tue, Feb 16, 2016 at 2:32 PM, Ian Romanick  wrote:
> On 02/16/2016 10:58 AM, Rob Clark wrote:
>> src/compiler/glsl/lower_discard_flow.cpp:79:1: warning: ‘ir_visitor_status 
>> {anonymous}::lower_discard_flow_visitor::visit_enter(ir_loop_jump*)’ defined 
>> but not used [-Wunused-function]
>>  lower_discard_flow_visitor::visit_enter(ir_loop_jump *ir)
>>  ^~
>>
>> Note, not sure if this was a latent bug?  Could be that was intended to
>> override visit(ir_loop_jump *)?
>
> I'll wager that is correct, and the bug has existed since day 1. :(

I guess this is as good an excuse as any to try to write a piglit test
(shader_test)..

BR,
-R

> To hit the bug, you'd need a loop with both a discard and a continue.  I
> suspect that is a rare combination.  To observe that the bug had been
> hit, you'd have to use a derivative (either via dFdx() and friends or
> texture()) in a particular way.  I'll have to try to think of a test case.
>
> It might be easier to just add a unit test.  We know that
>
>for (int i = 0; i < x; i++) {
>   if (z)
>  continue;
>
>   if (y)
>  discard;
>}
>
> should get transformed to
>
>for (int i = 0; i < x; i++) {
>   if (z) {
>  if (discarded)
> break;
>
>  continue;
>   }
>
>   if (y) {
>  discarded = true;
>  discard;
>   }
>
>   if (discarded)
>  break;
>}
>
>> Signed-off-by: Rob Clark 
>> ---
>>  src/compiler/glsl/lower_discard_flow.cpp | 12 
>>  1 file changed, 12 deletions(-)
>>
>> diff --git a/src/compiler/glsl/lower_discard_flow.cpp 
>> b/src/compiler/glsl/lower_discard_flow.cpp
>> index 9d0a56b..bdb96b4 100644
>> --- a/src/compiler/glsl/lower_discard_flow.cpp
>> +++ b/src/compiler/glsl/lower_discard_flow.cpp
>> @@ -63,7 +63,6 @@ public:
>> }
>>
>> ir_visitor_status visit_enter(ir_discard *ir);
>> -   ir_visitor_status visit_enter(ir_loop_jump *ir);
>> ir_visitor_status visit_enter(ir_loop *ir);
>> ir_visitor_status visit_enter(ir_function_signature *ir);
>>
>> @@ -76,17 +75,6 @@ public:
>>  } /* anonymous namespace */
>>
>>  ir_visitor_status
>> -lower_discard_flow_visitor::visit_enter(ir_loop_jump *ir)
>> -{
>> -   if (ir->mode != ir_loop_jump::jump_continue)
>> -  return visit_continue;
>> -
>> -   ir->insert_before(generate_discard_break());
>> -
>> -   return visit_continue;
>> -}
>> -
>> -ir_visitor_status
>>  lower_discard_flow_visitor::visit_enter(ir_discard *ir)
>>  {
>> ir_dereference *lhs = new(mem_ctx) ir_dereference_variable(discarded);
>>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 08/10] glsl: fix new gcc6 warnings

[Ian Romanick]

On 02/16/2016 10:58 AM, Rob Clark wrote:
> src/compiler/glsl/lower_discard_flow.cpp:79:1: warning: ‘ir_visitor_status 
> {anonymous}::lower_discard_flow_visitor::visit_enter(ir_loop_jump*)’ defined 
> but not used [-Wunused-function]
>  lower_discard_flow_visitor::visit_enter(ir_loop_jump *ir)
>  ^~
> 
> Note, not sure if this was a latent bug?  Could be that was intended to
> override visit(ir_loop_jump *)?

I'll wager that is correct, and the bug has existed since day 1. :(

To hit the bug, you'd need a loop with both a discard and a continue.  I
suspect that is a rare combination.  To observe that the bug had been
hit, you'd have to use a derivative (either via dFdx() and friends or
texture()) in a particular way.  I'll have to try to think of a test case.

It might be easier to just add a unit test.  We know that

   for (int i = 0; i < x; i++) {
  if (z)
 continue;

  if (y)
 discard;
   }

should get transformed to

   for (int i = 0; i < x; i++) {
  if (z) {
 if (discarded)
break;

 continue;
  }

  if (y) {
 discarded = true;
 discard;
  }

  if (discarded)
 break;
   }

> Signed-off-by: Rob Clark 
> ---
>  src/compiler/glsl/lower_discard_flow.cpp | 12 
>  1 file changed, 12 deletions(-)
> 
> diff --git a/src/compiler/glsl/lower_discard_flow.cpp 
> b/src/compiler/glsl/lower_discard_flow.cpp
> index 9d0a56b..bdb96b4 100644
> --- a/src/compiler/glsl/lower_discard_flow.cpp
> +++ b/src/compiler/glsl/lower_discard_flow.cpp
> @@ -63,7 +63,6 @@ public:
> }
>  
> ir_visitor_status visit_enter(ir_discard *ir);
> -   ir_visitor_status visit_enter(ir_loop_jump *ir);
> ir_visitor_status visit_enter(ir_loop *ir);
> ir_visitor_status visit_enter(ir_function_signature *ir);
>  
> @@ -76,17 +75,6 @@ public:
>  } /* anonymous namespace */
>  
>  ir_visitor_status
> -lower_discard_flow_visitor::visit_enter(ir_loop_jump *ir)
> -{
> -   if (ir->mode != ir_loop_jump::jump_continue)
> -  return visit_continue;
> -
> -   ir->insert_before(generate_discard_break());
> -
> -   return visit_continue;
> -}
> -
> -ir_visitor_status
>  lower_discard_flow_visitor::visit_enter(ir_discard *ir)
>  {
> ir_dereference *lhs = new(mem_ctx) ir_dereference_variable(discarded);
> 

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