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

[Nicolai Hähnle]

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?

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

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



> 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;
>           }
>        }
>