Re: [Mesa-dev] [PATCH 08/10] glsl: fix new gcc6 warnings
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
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
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
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
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
[Mesa-dev] [PATCH 08/10] glsl: fix new gcc6 warnings
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;
--
2.5.0
___
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
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
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
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
[Mesa-dev] [PATCH 08/10] glsl: fix new gcc6 warnings
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 *)?
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);
--
2.5.0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
