[Mesa-dev] [PATCH 1/2] nir/search: Don't match inexact expressions with exact subexpressions

[Jason Ekstrand]

On Tue, Mar 29, 2016 at 2:50 PM, Ian Romanick <idr at freedesktop.org> wrote:

> Does this fix any test cases?  Either way, this series is
>

No know test cases.  It does mean that we now correctly implement the
semantics required for SPIR-V


> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
>
> On 03/28/2016 04:54 PM, Jason Ekstrand wrote:
> > In the first pass of implementing exact handling, I made a mistake with
> > search-and-replace.  In particular, we only reallly handled exact/inexact
> > on the root of the tree.  Instead, we need to check every node in the
> tree
> > for an exact/inexact match.  As an example of this, consider the
> following
> > GLSL code
> >
> > precise float a = b + c;
> > if (a < 0) {
> >    do_stuff();
> > }
> >
> > In that case, only the add will be declared "exact" and an expression
> that
> > looks for "b + c < 0" will still match and replace it with "b < -c" which
> > may yield different results.  The solution is to simply bail if any of
> the
> > values are exact when matching an inexact expression.
> > ---
> >  src/compiler/nir/nir_search.c | 23 ++++++++++++++++++-----
> >  1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/compiler/nir/nir_search.c
> b/src/compiler/nir/nir_search.c
> > index 6e63063..b915101 100644
> > --- a/src/compiler/nir/nir_search.c
> > +++ b/src/compiler/nir/nir_search.c
> > @@ -28,6 +28,8 @@
> >  #include "nir_search.h"
> >
> >  struct match_state {
> > +   bool inexact_match;
> > +   bool has_exact_alu;
> >     unsigned variables_seen;
> >     nir_alu_src variables[NIR_SEARCH_MAX_VARIABLES];
> >  };
> > @@ -239,7 +241,10 @@ match_expression(const nir_search_expression *expr,
> nir_alu_instr *instr,
> >        return false;
> >
> >     assert(instr->dest.dest.is_ssa);
> > -   if (expr->inexact && instr->exact)
> > +
> > +   state->inexact_match = expr->inexact || state->inexact_match;
> > +   state->has_exact_alu = instr->exact || state->has_exact_alu;
> > +   if (state->inexact_match && state->has_exact_alu)
> >        return false;
> >
> >     assert(!instr->dest.saturate);
> > @@ -410,7 +415,7 @@ bitsize_tree_filter_down(bitsize_tree *tree,
> unsigned size)
> >
> >  static nir_alu_src
> >  construct_value(const nir_search_value *value,
> > -                unsigned num_components, bitsize_tree *bitsize, bool
> exact,
> > +                unsigned num_components, bitsize_tree *bitsize,
> >                  struct match_state *state,
> >                  nir_instr *instr, void *mem_ctx)
> >  {
> > @@ -424,10 +429,16 @@ construct_value(const nir_search_value *value,
> >        nir_alu_instr *alu = nir_alu_instr_create(mem_ctx, expr->opcode);
> >        nir_ssa_dest_init(&alu->instr, &alu->dest.dest, num_components,
> >                          bitsize->dest_size, NULL);
> > -      alu->exact = exact;
> >        alu->dest.write_mask = (1 << num_components) - 1;
> >        alu->dest.saturate = false;
> >
> > +      /* We have no way of knowing what values in a given search
> expression
> > +       * map to a particular replacement value.  Therefore, if the
> > +       * expression we are replacing has any exact values, the entire
> > +       * replacement should be exact.
> > +       */
> > +      alu->exact = state->has_exact_alu;
> > +
> >        for (unsigned i = 0; i < nir_op_infos[expr->opcode].num_inputs;
> i++) {
> >           /* If the source is an explicitly sized source, then we need
> to reset
> >            * the number of components to match.
> > @@ -436,7 +447,7 @@ construct_value(const nir_search_value *value,
> >              num_components = nir_op_infos[alu->op].input_sizes[i];
> >
> >           alu->src[i] = construct_value(expr->srcs[i],
> > -                                       num_components,
> bitsize->srcs[i], exact,
> > +                                       num_components, bitsize->srcs[i],
> >                                         state, instr, mem_ctx);
> >        }
> >
> > @@ -546,6 +557,8 @@ nir_replace_instr(nir_alu_instr *instr, const
> nir_search_expression *search,
> >     assert(instr->dest.dest.is_ssa);
> >
> >     struct match_state state;
> > +   state.inexact_match = false;
> > +   state.has_exact_alu = false;
> >     state.variables_seen = 0;
> >
> >     if (!match_expression(search, instr,
> instr->dest.dest.ssa.num_components,
> > @@ -569,7 +582,7 @@ nir_replace_instr(nir_alu_instr *instr, const
> nir_search_expression *search,
> >
> >     mov->src[0] = construct_value(replace,
> >                                   instr->dest.dest.ssa.num_components,
> tree,
> > -                                 instr->exact, &state, &instr->instr,
> mem_ctx);
> > +                                 &state, &instr->instr, mem_ctx);
> >     nir_instr_insert_before(&instr->instr, &mov->instr);
> >
> >     nir_ssa_def_rewrite_uses(&instr->dest.dest.ssa,
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160329/4a0371ad/attachment.html>