[Mesa-dev] [PATCH] glsl: fix opt_minmax redundancy checks against baserange

[Timothy Arceri]

On Fri, 2017-01-06 at 12:36 +0100, Nicolai Hähnle wrote:
> On 06.01.2017 00:26, Timothy Arceri wrote:
> > Marking operations as redundant if they are equal to the base
> > range is fine when the tree structure is something like this:
> > 
> >         max
> >       /     \
> >      max     b
> >     /   \
> >    3    max
> >        /   \
> >       3     a
> > 
> > But the opt falls apart with a tree like this:
> > 
> >         max
> >      /       \
> >     max     max
> >    /   \   /   \
> >   3    a   b    3
> 
> This will just become max(a, b), right?

Ah yes so it does. I spent so much time trying to reproduce the problem
(mostly trying trees like the first one) that I though it was doing
something more crazy than that. Makes much more sense looking at it
again with less tired eyes.

> 
> The problem is that both branches are treated the same: descending
> in 
> the left branch will prune the constant, and then descending the
> right 
> branch will prune the constant there as well, because limits[0]
> wasn't 
> updated to take the change on the left branch into account, and so
> we 
> still get [3,\infty) as baserange.
> 
> With your change, nothing will be done at all. That's fine as far as
> I'm 
> concerned. I don't see a clean way of updating the limits on-the-fly.
> 
> Thanks for the piglit test. An explanatory comment would be nice,
> but 
> either way, this patch is

I'll add your explanation to the commit message.

> 
> Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>
> 
> 

Thanks :)

> 
> > I'm not 100% sure what is going wrong as a tree like:
> > 
> >         max
> >      /       \
> >     max     max
> >    /   \   /   \
> >   3    a   b    4
> > 
> > Will remove the right hand side max just fine. But not marking
> > limits that equal the base range seems to fix the problem.
> > 
> > NIR algebraic opt will clean up the first tree for anyway,
> > hopefully
> > other backends are smart enough to do this also.
> > 
> > Cc: "13.0" <mesa-stable at lists.freedesktop.org>
> > ---
> >  src/compiler/glsl/opt_minmax.cpp | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/compiler/glsl/opt_minmax.cpp
> > b/src/compiler/glsl/opt_minmax.cpp
> > index 29482ee..9f64db9 100644
> > --- a/src/compiler/glsl/opt_minmax.cpp
> > +++ b/src/compiler/glsl/opt_minmax.cpp
> > @@ -355,7 +355,7 @@
> > ir_minmax_visitor::prune_expression(ir_expression *expr,
> > minmax_range baserange)
> >            */
> >           if (!is_redundant && limits[i].low && baserange.high) {
> >              cr = compare_components(limits[i].low,
> > baserange.high);
> > -            if (cr >= EQUAL && cr != MIXED)
> > +            if (cr > EQUAL && cr != MIXED)
> >                 is_redundant = true;
> >           }
> >        } else {
> > @@ -373,7 +373,7 @@
> > ir_minmax_visitor::prune_expression(ir_expression *expr,
> > minmax_range baserange)
> >            */
> >           if (!is_redundant && limits[i].high && baserange.low) {
> >              cr = compare_components(limits[i].high,
> > baserange.low);
> > -            if (cr <= EQUAL)
> > +            if (cr < EQUAL)
> >                 is_redundant = true;
> >           }
> >        }
> > 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev