Re: [Mesa-dev] [PATCH] glsl: remove unreachable assert()

[Timothy Arceri]

On 11/04/18 20:50, Emil Velikov wrote:

On 10 April 2018 at 18:10, Jason Ekstrand  wrote:

On Tue, Apr 10, 2018 at 10:05 AM, Emil Velikov 
wrote:

On 10 April 2018 at 17:53, Ivan Kalvachev  wrote:

On 3/28/18, Emil Velikov  wrote:

From: Emil Velikov 

Earlier commit enforced that we'll bail out if the number of
terminators
is different than 2. With that in mind, the assert() will never
trigger.

Fixes: 56b867395de ("glsl: fix infinite loop caused by bug in loop
unrolling pass")


This doesn't fix anything.  Not triggering an assert is not a bug.

Removing a bogus assert that does get triggered by perfectly valid
code-paths would be a bug fix.


Cc: Timothy Arceri 
Signed-off-by: Emil Velikov 

Just a nitpick.
The explanations doesn't sound right to me.

Asserts are meant to never trigger.
They are used to check the internal logic of the code.

If this assert does trigger that would mean
that there is a bug in the code that makes sure
the number of terminators is different than 2.

It is better to catch bug with assert than
to silently do something wrong.


Right. wording is not perfect. As-is the assert provides misleading
assumption considering the explicit check


What misleading information would that be?  In this particular case, we have
multiple cases of "if (term_count == 1) { ... } else { ... }"  so knowing
that term_count never goes above 2 is useful.  How is "assert(term_count <
2)" misleading?

In general, the point of asserts is to declare assumptions made by the code
that follows.  This serves both as documentation to developers and ensures
that we find out if those assumptions are ever violated.

There is _explicit_ length check just before the loop. It should be
self-documenting enough, right?

if (... || ls->terminators.length() != 2)
 return visit_continue;

That code was added with Tim's commit. I'm yet to see anybody adding
asserts for checking loop boundaries, hence the misleading label.
The better part is the existing assert did not flag prior to Tim's fix
(see the commit and bugreport).

Either way - if you feel strongly about it just revert it.


I'm fine with this being removed. We don't need an assert to check that 
the if above succeeded. Also I wrote a piglit test to pick up the bug 
that was fixed, so I'm not worried about regressions.




HTH
Emil


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

Re: [Mesa-dev] [PATCH] glsl: remove unreachable assert()

[Ivan Kalvachev]

On 4/11/18, Ivan Kalvachev  wrote:
> On 4/11/18, Emil Velikov  wrote:
>> On 10 April 2018 at 18:10, Jason Ekstrand  wrote:
>>> On Tue, Apr 10, 2018 at 10:05 AM, Emil Velikov
>>> 
>>> wrote:

 On 10 April 2018 at 17:53, Ivan Kalvachev  wrote:
 > On 3/28/18, Emil Velikov  wrote:
 >> From: Emil Velikov 
 >>
 >> Earlier commit enforced that we'll bail out if the number of
 >> terminators
 >> is different than 2. With that in mind, the assert() will never
 >> trigger.
 >>
 >> Fixes: 56b867395de ("glsl: fix infinite loop caused by bug in loop
 >> unrolling pass")
>>>
>>>
>>> This doesn't fix anything.  Not triggering an assert is not a bug.
>>>
>>> Removing a bogus assert that does get triggered by perfectly valid
>>> code-paths would be a bug fix.
>>>

 >> Cc: Timothy Arceri 
 >> Signed-off-by: Emil Velikov 
 >
 > Just a nitpick.
 > The explanations doesn't sound right to me.
 >
 > Asserts are meant to never trigger.
 > They are used to check the internal logic of the code.
 >
 > If this assert does trigger that would mean
 > that there is a bug in the code that makes sure
 > the number of terminators is different than 2.
 >
 > It is better to catch bug with assert than
 > to silently do something wrong.
 >
 Right. wording is not perfect. As-is the assert provides misleading
 assumption considering the explicit check
>>>
>>>
>>> What misleading information would that be?  In this particular case, we
>>> have
>>> multiple cases of "if (term_count == 1) { ... } else { ... }"  so
>>> knowing
>>> that term_count never goes above 2 is useful.  How is "assert(term_count
>>> <
>>> 2)" misleading?
>>>
>>> In general, the point of asserts is to declare assumptions made by the
>>> code
>>> that follows.  This serves both as documentation to developers and
>>> ensures
>>> that we find out if those assumptions are ever violated.
>>
>> There is _explicit_ length check just before the loop. It should be
>> self-documenting enough, right?
>>
>>if (... || ls->terminators.length() != 2)
>> return visit_continue;
>>
>> That code was added with Tim's commit. I'm yet to see anybody adding
>> asserts for checking loop boundaries, hence the misleading label.
>> The better part is the existing assert did not flag prior to Tim's fix
>> (see the commit and bugreport).
>>
>> Either way - if you feel strongly about it just revert it.
>
> I see the root of the confusion.
>
> You see ( x !=2 ) and (  !(x<2) ) are not equivalent.
>
> Maybe the explicit check should be
> if ( x >= 2 ) return;

Yff, the check makes sure the x is always 2.

I should not write mails before morning cafe.

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

Re: [Mesa-dev] [PATCH] glsl: remove unreachable assert()

[Ivan Kalvachev]

On 4/11/18, Emil Velikov  wrote:
> On 10 April 2018 at 18:10, Jason Ekstrand  wrote:
>> On Tue, Apr 10, 2018 at 10:05 AM, Emil Velikov 
>> wrote:
>>>
>>> On 10 April 2018 at 17:53, Ivan Kalvachev  wrote:
>>> > On 3/28/18, Emil Velikov  wrote:
>>> >> From: Emil Velikov 
>>> >>
>>> >> Earlier commit enforced that we'll bail out if the number of
>>> >> terminators
>>> >> is different than 2. With that in mind, the assert() will never
>>> >> trigger.
>>> >>
>>> >> Fixes: 56b867395de ("glsl: fix infinite loop caused by bug in loop
>>> >> unrolling pass")
>>
>>
>> This doesn't fix anything.  Not triggering an assert is not a bug.
>>
>> Removing a bogus assert that does get triggered by perfectly valid
>> code-paths would be a bug fix.
>>
>>>
>>> >> Cc: Timothy Arceri 
>>> >> Signed-off-by: Emil Velikov 
>>> >
>>> > Just a nitpick.
>>> > The explanations doesn't sound right to me.
>>> >
>>> > Asserts are meant to never trigger.
>>> > They are used to check the internal logic of the code.
>>> >
>>> > If this assert does trigger that would mean
>>> > that there is a bug in the code that makes sure
>>> > the number of terminators is different than 2.
>>> >
>>> > It is better to catch bug with assert than
>>> > to silently do something wrong.
>>> >
>>> Right. wording is not perfect. As-is the assert provides misleading
>>> assumption considering the explicit check
>>
>>
>> What misleading information would that be?  In this particular case, we
>> have
>> multiple cases of "if (term_count == 1) { ... } else { ... }"  so knowing
>> that term_count never goes above 2 is useful.  How is "assert(term_count
>> <
>> 2)" misleading?
>>
>> In general, the point of asserts is to declare assumptions made by the
>> code
>> that follows.  This serves both as documentation to developers and
>> ensures
>> that we find out if those assumptions are ever violated.
>
> There is _explicit_ length check just before the loop. It should be
> self-documenting enough, right?
>
>if (... || ls->terminators.length() != 2)
> return visit_continue;
>
> That code was added with Tim's commit. I'm yet to see anybody adding
> asserts for checking loop boundaries, hence the misleading label.
> The better part is the existing assert did not flag prior to Tim's fix
> (see the commit and bugreport).
>
> Either way - if you feel strongly about it just revert it.

I see the root of the confusion.

You see ( x !=2 ) and (  !(x<2) ) are not equivalent.

Maybe the explicit check should be
if ( x >= 2 ) return;


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

Re: [Mesa-dev] [PATCH] glsl: remove unreachable assert()

[Emil Velikov]

On 10 April 2018 at 18:10, Jason Ekstrand  wrote:
> On Tue, Apr 10, 2018 at 10:05 AM, Emil Velikov 
> wrote:
>>
>> On 10 April 2018 at 17:53, Ivan Kalvachev  wrote:
>> > On 3/28/18, Emil Velikov  wrote:
>> >> From: Emil Velikov 
>> >>
>> >> Earlier commit enforced that we'll bail out if the number of
>> >> terminators
>> >> is different than 2. With that in mind, the assert() will never
>> >> trigger.
>> >>
>> >> Fixes: 56b867395de ("glsl: fix infinite loop caused by bug in loop
>> >> unrolling pass")
>
>
> This doesn't fix anything.  Not triggering an assert is not a bug.
>
> Removing a bogus assert that does get triggered by perfectly valid
> code-paths would be a bug fix.
>
>>
>> >> Cc: Timothy Arceri 
>> >> Signed-off-by: Emil Velikov 
>> >
>> > Just a nitpick.
>> > The explanations doesn't sound right to me.
>> >
>> > Asserts are meant to never trigger.
>> > They are used to check the internal logic of the code.
>> >
>> > If this assert does trigger that would mean
>> > that there is a bug in the code that makes sure
>> > the number of terminators is different than 2.
>> >
>> > It is better to catch bug with assert than
>> > to silently do something wrong.
>> >
>> Right. wording is not perfect. As-is the assert provides misleading
>> assumption considering the explicit check
>
>
> What misleading information would that be?  In this particular case, we have
> multiple cases of "if (term_count == 1) { ... } else { ... }"  so knowing
> that term_count never goes above 2 is useful.  How is "assert(term_count <
> 2)" misleading?
>
> In general, the point of asserts is to declare assumptions made by the code
> that follows.  This serves both as documentation to developers and ensures
> that we find out if those assumptions are ever violated.

There is _explicit_ length check just before the loop. It should be
self-documenting enough, right?

   if (... || ls->terminators.length() != 2)
return visit_continue;

That code was added with Tim's commit. I'm yet to see anybody adding
asserts for checking loop boundaries, hence the misleading label.
The better part is the existing assert did not flag prior to Tim's fix
(see the commit and bugreport).

Either way - if you feel strongly about it just revert it.

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

Re: [Mesa-dev] [PATCH] glsl: remove unreachable assert()

[Jason Ekstrand]

On Tue, Apr 10, 2018 at 10:05 AM, Emil Velikov 
wrote:

> On 10 April 2018 at 17:53, Ivan Kalvachev  wrote:
> > On 3/28/18, Emil Velikov  wrote:
> >> From: Emil Velikov 
> >>
> >> Earlier commit enforced that we'll bail out if the number of terminators
> >> is different than 2. With that in mind, the assert() will never trigger.
> >>
> >> Fixes: 56b867395de ("glsl: fix infinite loop caused by bug in loop
> >> unrolling pass")
>

This doesn't fix anything.  Not triggering an assert is not a bug.

Removing a bogus assert that does get triggered by perfectly valid
code-paths would be a bug fix.


> >> Cc: Timothy Arceri 
> >> Signed-off-by: Emil Velikov 
> >
> > Just a nitpick.
> > The explanations doesn't sound right to me.
> >
> > Asserts are meant to never trigger.
> > They are used to check the internal logic of the code.
> >
> > If this assert does trigger that would mean
> > that there is a bug in the code that makes sure
> > the number of terminators is different than 2.
> >
> > It is better to catch bug with assert than
> > to silently do something wrong.
> >
> Right. wording is not perfect. As-is the assert provides misleading
> assumption considering the explicit check


What misleading information would that be?  In this particular case, we
have multiple cases of "if (term_count == 1) { ... } else { ... }"  so
knowing that term_count never goes above 2 is useful.  How is
"assert(term_count < 2)" misleading?

In general, the point of asserts is to declare assumptions made by the code
that follows.  This serves both as documentation to developers and ensures
that we find out if those assumptions are ever violated.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] glsl: remove unreachable assert()

[Emil Velikov]

On 10 April 2018 at 17:53, Ivan Kalvachev  wrote:
> On 3/28/18, Emil Velikov  wrote:
>> From: Emil Velikov 
>>
>> Earlier commit enforced that we'll bail out if the number of terminators
>> is different than 2. With that in mind, the assert() will never trigger.
>>
>> Fixes: 56b867395de ("glsl: fix infinite loop caused by bug in loop
>> unrolling pass")
>> Cc: Timothy Arceri 
>> Signed-off-by: Emil Velikov 
>
> Just a nitpick.
> The explanations doesn't sound right to me.
>
> Asserts are meant to never trigger.
> They are used to check the internal logic of the code.
>
> If this assert does trigger that would mean
> that there is a bug in the code that makes sure
> the number of terminators is different than 2.
>
> It is better to catch bug with assert than
> to silently do something wrong.
>
Right. wording is not perfect. As-is the assert provides misleading
assumption considering the explicit check.

> Also, sometimes compilers might use
> the assert assumptions to optimize the code.
> (Even when the assertion itself is disabled.)
>
Fully aware of that, yet i doubt it will matter in this case.
If you want to give it a check, that would be appreciated.

JFYI I've pushed this ~2h before your reply. But if people prefer I
can revert it.

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

Re: [Mesa-dev] [PATCH] glsl: remove unreachable assert()

[Ivan Kalvachev]

On 3/28/18, Emil Velikov  wrote:
> From: Emil Velikov 
>
> Earlier commit enforced that we'll bail out if the number of terminators
> is different than 2. With that in mind, the assert() will never trigger.
>
> Fixes: 56b867395de ("glsl: fix infinite loop caused by bug in loop
> unrolling pass")
> Cc: Timothy Arceri 
> Signed-off-by: Emil Velikov 

Just a nitpick.
The explanations doesn't sound right to me.

Asserts are meant to never trigger.
They are used to check the internal logic of the code.

If this assert does trigger that would mean
that there is a bug in the code that makes sure
the number of terminators is different than 2.

It is better to catch bug with assert than
to silently do something wrong.

Also, sometimes compilers might use
the assert assumptions to optimize the code.
(Even when the assertion itself is disabled.)

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

Re: [Mesa-dev] [PATCH] glsl: remove unreachable assert()

[Timothy Arceri]

Reviewed-by: Timothy Arceri 

On 29/03/18 04:25, Emil Velikov wrote:

From: Emil Velikov 

Earlier commit enforced that we'll bail out if the number of terminators
is different than 2. With that in mind, the assert() will never trigger.

Fixes: 56b867395de ("glsl: fix infinite loop caused by bug in loop
unrolling pass")
Cc: Timothy Arceri 
Signed-off-by: Emil Velikov 
---
  src/compiler/glsl/loop_unroll.cpp | 2 --
  1 file changed, 2 deletions(-)

diff --git a/src/compiler/glsl/loop_unroll.cpp 
b/src/compiler/glsl/loop_unroll.cpp
index f6efe6475a..874f418568 100644
--- a/src/compiler/glsl/loop_unroll.cpp
+++ b/src/compiler/glsl/loop_unroll.cpp
@@ -528,8 +528,6 @@ loop_unroll_visitor::visit_leave(ir_loop *ir)
 unsigned term_count = 0;
 bool first_term_then_continue = false;
 foreach_in_list(loop_terminator, t, >terminators) {
-  assert(term_count < 2);
-
ir_if *ir_if = t->ir->as_if();
assert(ir_if != NULL);
  


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

[Mesa-dev] [PATCH] glsl: remove unreachable assert()

[Emil Velikov]

From: Emil Velikov 

Earlier commit enforced that we'll bail out if the number of terminators
is different than 2. With that in mind, the assert() will never trigger.

Fixes: 56b867395de ("glsl: fix infinite loop caused by bug in loop
unrolling pass")
Cc: Timothy Arceri 
Signed-off-by: Emil Velikov 
---
 src/compiler/glsl/loop_unroll.cpp | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/compiler/glsl/loop_unroll.cpp 
b/src/compiler/glsl/loop_unroll.cpp
index f6efe6475a..874f418568 100644
--- a/src/compiler/glsl/loop_unroll.cpp
+++ b/src/compiler/glsl/loop_unroll.cpp
@@ -528,8 +528,6 @@ loop_unroll_visitor::visit_leave(ir_loop *ir)
unsigned term_count = 0;
bool first_term_then_continue = false;
foreach_in_list(loop_terminator, t, >terminators) {
-  assert(term_count < 2);
-
   ir_if *ir_if = t->ir->as_if();
   assert(ir_if != NULL);
 
-- 
2.16.0

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