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

[Timothy Arceri]

On Fri, 2017-01-06 at 13:29 +0100, Iago Toral wrote:
> Hi Tim,
> 
> it's been a while, so off the top of my head I don't have any
> particular suggestions or the capacity to tell whether your fix is
> correct or not :-/.
> 
> I'll try to spend some time re-acquainting myself with that code on
> Monday and see if I can see what is going on here.

Not a problem. I'm going to push this patch with Nicolai's review so
feel free to ignore or work on a fix that properly removes the
redundant node up to you. It's probably not worth spending much time on
GLSL IR opts these days though.

> 
> Iago
> 
> On Fri, 2017-01-06 at 22:08 +1100, Timothy Arceri wrote:
> > Hi Iago/Petri,
> > 
> > Just Ccing you guys in case you have a better solution (I know its
> > been
> > a while since this was written).
> > 
> > This is causing incorrect shaders in at least Serious Sam 3
> > possibly
> > others. I've sent some piglit tests to reproduce it to the piglit
> > list.
> > 
> > Thanks,
> > Tim
> > 
> > On Fri, 2017-01-06 at 10:26 +1100, 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
> > > 
> > > 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