<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Mar 29, 2016 at 2:50 PM, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Does this fix any test cases? Either way, this series is </blockquote><div> </div><div>No know test cases. It does mean that we now correctly implement the semantics required for SPIR-V </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Reviewed-by: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com">ian.d.romanick@intel.com</a>> <div class="HOEnZb"><div class="h5"> 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, > </div></div></blockquote></div> </div></div>