On Mar 22, 2015 8:48 PM, "Connor Abbott" <<a href="mailto:cwabbott0@gmail.com">cwabbott0@gmail.com</a>> wrote: > > On Fri, Mar 20, 2015 at 5:23 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote: > > v2: Fix the spelling of analyze and re-arrange code for better readability > > as per Connor's comments. > > v3: Make the naming of things more consistent and add a pile of comments > > --- > > src/mesa/drivers/dri/i965/Makefile.sources | 2 + > > src/mesa/drivers/dri/i965/brw_nir.h | 78 ++++++ > > .../dri/i965/brw_nir_analyze_boolean_resolves.c | 275 +++++++++++++++++++++ > > 3 files changed, 355 insertions(+) > > create mode 100644 src/mesa/drivers/dri/i965/brw_nir.h > > create mode 100644 src/mesa/drivers/dri/i965/brw_nir_analyze_boolean_resolves.c > > > > diff --git a/src/mesa/drivers/dri/i965/Makefile.sources b/src/mesa/drivers/dri/i965/Makefile.sources > > index c69441b..3a3df70 100644 > > --- a/src/mesa/drivers/dri/i965/Makefile.sources > > +++ b/src/mesa/drivers/dri/i965/Makefile.sources > > @@ -73,6 +73,8 @@ i965_FILES = \ > > brw_meta_util.h \ > > brw_misc_state.c \ > > brw_multisample_state.h \ > > + brw_nir.h \ > > + brw_nir_analyze_boolean_resolves.c \ > > brw_object_purgeable.c \ > > brw_packed_float.c \ > > brw_performance_monitor.c \ > > diff --git a/src/mesa/drivers/dri/i965/brw_nir.h b/src/mesa/drivers/dri/i965/brw_nir.h > > new file mode 100644 > > index 0000000..27782a3 > > --- /dev/null > > +++ b/src/mesa/drivers/dri/i965/brw_nir.h > > @@ -0,0 +1,78 @@ > > +/* > > + * Copyright © 2015 Intel Corporation > > + * > > + * Permission is hereby granted, free of charge, to any person obtaining a > > + * copy of this software and associated documentation files (the "Software"), > > + * to deal in the Software without restriction, including without limitation > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > > + * and/or sell copies of the Software, and to permit persons to whom the > > + * Software is furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice (including the next > > + * paragraph) shall be included in all copies or substantial portions of the > > + * Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > > + * IN THE SOFTWARE. > > + */ > > + > > +#pragma once > > + > > +#include "glsl/nir/nir.h" > > + > > +#ifdef __cplusplus > > +extern "C" { > > +#endif > > + > > +/* Flags set in the instr->pass_flags field by i965 analysis passes */ > > +enum { > > + BRW_NIR_NON_BOOLEAN = 0x0, > > + > > + /* Indicates that the given instruction's destination is a boolean > > + * value but that it needs to be resolved before it can be used. > > + * On Gen <= 5, CMP instructions return a 32-bit value where the bottom > > + * bit represents the actual true/false value of the compare and the top > > + * 31 bits are undefined. In order to use this value, we have to do a > > + * "resolve" operation by replacing the value of the CMP with -(x & 1) > > + * to sign-extend the bottom bit to 0/~0. > > + */ > > + BRW_NIR_BOOLEAN_NEEDS_RESOLVE = 0x1, > > + > > + /* Indicates that the given instruction's destination is a boolean > > + * value that has intentionally been left unresolved. Not all boolean > > + * values need to be resolved immediately. For instance, if we have > > + * > > + * CMP r1 r2 r3 > > + * CMP r4 r5 r6 > > + * AND r7 r1 r4 > > + * > > + * We don't have to resolve the result of the two CMP instructions > > + * immediately because the AND still does an AND of the bottom bits. > > + * Instead, we can save ourselves instructions by delaying the resolve > > + * until after the AND. The result of the two CMP instructions is left > > + * as BRW_NIR_BOOLEAN_UNRESOLVED. > > + */ > > + BRW_NIR_BOOLEAN_UNRESOLVED = 0x2, > > + > > + /* Indicates a that the given instruction's destination is a boolean > > + * value that does not need a resolve. For instance, if you AND two > > + * values that are BRW_NIR_BOOLEAN_NEEDS_RESOLVE then we know that both > > + * values will be 0/~0 before we get them and the result of the AND is > > + * also guaranteed to be 0/~0 and does not need a resolve. > > + */ > > + BRW_NIR_BOOLEAN_NO_RESOLVE = 0x3, > > + > > + /* A mask to mask the boolean status values off of instr->pass_flags */ > > + BRW_NIR_BOOLEAN_MASK = 0x3, > > +}; > > + > > +void brw_nir_analyze_boolean_resolves(nir_shader *nir); > > + > > +#ifdef __cplusplus > > +} > > +#endif > > diff --git a/src/mesa/drivers/dri/i965/brw_nir_analyze_boolean_resolves.c b/src/mesa/drivers/dri/i965/brw_nir_analyze_boolean_resolves.c > > new file mode 100644 > > index 0000000..e893a65 > > --- /dev/null > > +++ b/src/mesa/drivers/dri/i965/brw_nir_analyze_boolean_resolves.c > > @@ -0,0 +1,275 @@ > > +/* > > + * Copyright © 2015 Intel Corporation > > + * > > + * Permission is hereby granted, free of charge, to any person obtaining a > > + * copy of this software and associated documentation files (the "Software"), > > + * to deal in the Software without restriction, including without limitation > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense, > > + * and/or sell copies of the Software, and to permit persons to whom the > > + * Software is furnished to do so, subject to the following conditions: > > + * > > + * The above copyright notice and this permission notice (including the next > > + * paragraph) shall be included in all copies or substantial portions of the > > + * Software. > > + * > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS > > + * IN THE SOFTWARE. > > + * > > + * Authors: > > + * Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> > > + */ > > + > > +#include "brw_nir.h" > > + > > +/* > > + * This file implements an analysis pass that determines when we have to do > > + * a boolean resolve on Gen <= 5. Instructions that need a boolean resolve > > + * will have the booleans portion of the instr->pass_flags field set to > > + * BRW_NIR_BOOLEAN_NEEDS_RESOLVE. > > + */ > > + > > + > > +/** Returns the resolve status for the given source > > + * > > + * If the source has a parent instruction then the resolve status is the > > + * status of the parent instruction. If the source does not have a parent > > + * instruction then we don't know so we return NON_BOOLEAN. > > + */ > > +static uint8_t > > +get_resolve_status_for_src(nir_src *src) > > +{ > > + nir_instr *src_instr; > > + if (src->is_ssa) { > > + src_instr = src->ssa->parent_instr; > > + } else { > > + src_instr = src->reg.reg->parent_instr; > > + } > > + > > + if (src_instr) { > > + uint8_t resolve_status = src_instr->pass_flags & BRW_NIR_BOOLEAN_MASK; > > + > > + /* If the source instruction needs resolve, then from the perspective > > + * of the user, it's a true boolean. > > + */ > > + if (resolve_status == BRW_NIR_BOOLEAN_NEEDS_RESOLVE) > > + resolve_status = BRW_NIR_BOOLEAN_NO_RESOLVE; > > + return resolve_status; > > + } else { > > + return BRW_NIR_NON_BOOLEAN; > > + } > > +} > > + > > +/** Marks the given source as needing a resolve > > + * > > + * If the given source corresponds to an unresolved boolean it marks it as > > + * needing a resolve. Otherwise, we leave it alone. > > + */ > > +static bool > > +src_mark_needs_resolve(nir_src *src, void *void_state) > > +{ > > + nir_instr *src_instr; > > + if (src->is_ssa) { > > + src_instr = src->ssa->parent_instr; > > + } else { > > + src_instr = src->reg.reg->parent_instr; > > + } > > + > > + if (src_instr) { > > + uint8_t resolve_status = src_instr->pass_flags & BRW_NIR_BOOLEAN_MASK; > > + > > + /* If the source instruction is unresolved, then mark it as needing > > + * to be resolved. > > + */ > > + if (resolve_status == BRW_NIR_BOOLEAN_UNRESOLVED) { > > + src_instr->pass_flags &= ~BRW_NIR_BOOLEAN_MASK; > > + src_instr->pass_flags |= BRW_NIR_BOOLEAN_NEEDS_RESOLVE; > > + } > > + > > + } > > + > > + return true; > > +} > > + > > +static bool > > +analyze_boolean_resolves_block(nir_block *block, void *void_state) > > +{ > > + nir_foreach_instr(block, instr) { > > + switch (instr->type) { > > + case nir_instr_type_alu: { > > + /* For ALU instructions, the resolve status is handled in a > > + * three-step process. > > + * > > + * 1) Look at the instruction type and sources and determine if it > > + * can be left unresolved. > > + * > > + * 2) Look at the destination and see if we have to resolve > > + * anyway. (This is the case if this instruction is not the > > + * only instruction writing to a given register.) > > + * > > + * 3) If the instruction has a resolve status other than > > + * BOOL_UNRESOLVED or BOOL_NEEDS_RESOLVE then we walk through > > + * the sources and ensure that they are also resolved. This > > + * ensures that we don't end up with any stray unresolved > > + * booleans going into ADDs or something like that. > > + */ > > + > > + uint8_t resolve_status; > > + nir_alu_instr *alu = nir_instr_as_alu(instr); > > + switch (alu->op) { > > + case nir_op_flt: > > + case nir_op_ilt: > > + case nir_op_ult: > > + case nir_op_fge: > > + case nir_op_ige: > > + case nir_op_uge: > > + case nir_op_feq: > > + case nir_op_ieq: > > + case nir_op_fne: > > + case nir_op_ine: > > + case nir_op_f2b: > > + case nir_op_i2b: > > + /* This instruction will turn into a CMP when we actually emit > > + * so the result will have to be resolved before it can be used. > > + */ > > + resolve_status = BRW_NIR_BOOLEAN_UNRESOLVED; > > + > > + /* Even though the destination is allowed to be left unresolved, > > + * the sources are treated as regular integers or floats so > > + * they need to be resolved. > > + */ > > + nir_foreach_src(instr, src_mark_needs_resolve, NULL); > > + break; > > + > > + case nir_op_imov: > > + case nir_op_inot: > > + if (alu->dest.write_mask == 1) { > > + /* This is a single-source instruction. Just copy the > > + * resolve status from the source. > > + */ > > + resolve_status = get_resolve_status_for_src(&alu->src[0].src); > > + } else { > > + /* Vector versions will be assumed to be non-boolean. */ > > + resolve_status = BRW_NIR_NON_BOOLEAN; > > Even though all non-vector things are currently lowered via > lower_alu_to_scalar so it won't matter until we add NIR -> vec4 > support, I think we should delete this if-statement and always go with > the first case -- you're just being too conservative, and it's less > code :) > > > + } > > + break; > > + > > + case nir_op_iand: > > + case nir_op_ior: > > + case nir_op_ixor: { > > + assert(alu->dest.write_mask == 1); > > Similarly, I think vector and/or/etc should "just work" with this > code. I know this is fixing a case that we don't hit yet, but given it > only involves deleting code I think it still makes sense to do it now. > Otherwise, this patch is Sure, I can do that. Most of that was added as I was trying to figure out how these would interact. No reason not to just let it work on vectors. I'll get rid of the pointless code. --Jason > Reviewed-by: Connor Abbott <<a href="mailto:cwabbott0@gmail.com">cwabbott0@gmail.com</a>> > > > + > > + uint8_t src0_status = get_resolve_status_for_src(&alu->src[0].src); > > + uint8_t src1_status = get_resolve_status_for_src(&alu->src[1].src); > > + > > + if (src0_status == src1_status) { > > + resolve_status = src0_status; > > + } else if (src0_status == BRW_NIR_NON_BOOLEAN || > > + src1_status == BRW_NIR_NON_BOOLEAN) { > > + /* If one of the sources is a non-boolean then the whole > > + * thing is a non-boolean. > > + */ > > + resolve_status = BRW_NIR_NON_BOOLEAN; > > + } else { > > + /* At this point one of them is a true boolean and one is a > > + * boolean that needs a resolve. We could either resolve the > > + * unresolved source or we could resolve here. If we resolve > > + * the unresolved source then we get two resolves for the price > > + * of one. Just set this one to BOOLEAN_NO_RESOLVE and we'll > > + * let the code below force a resolve on the unresolved source. > > + */ > > + resolve_status = BRW_NIR_BOOLEAN_NO_RESOLVE; > > + } > > + break; > > + } > > + > > + default: > > + resolve_status = BRW_NIR_NON_BOOLEAN; > > + } > > + > > + /* If the destination is SSA-like, go ahead allow unresolved booleans. > > + * If the destination register doesn't have a well-defined parent_instr > > + * we need to resolve immediately. > > + */ > > + if (alu->dest.dest.reg.reg->parent_instr == NULL && > > + resolve_status == BRW_NIR_BOOLEAN_UNRESOLVED) { > > + resolve_status = BRW_NIR_BOOLEAN_NEEDS_RESOLVE; > > + } > > + > > + instr->pass_flags = (instr->pass_flags & ~BRW_NIR_BOOLEAN_MASK) | > > + resolve_status; > > + > > + /* Finally, resolve sources if it's needed */ > > + switch (resolve_status) { > > + case BRW_NIR_BOOLEAN_NEEDS_RESOLVE: > > + case BRW_NIR_BOOLEAN_UNRESOLVED: > > + /* This instruction is either unresolved or we're doing the > > + * resolve here; leave the sources alone. > > + */ > > + break; > > + > > + case BRW_NIR_BOOLEAN_NO_RESOLVE: > > + case BRW_NIR_NON_BOOLEAN: > > + nir_foreach_src(instr, src_mark_needs_resolve, NULL); > > + break; > > + > > + default: > > + unreachable("Invalid boolean flag"); > > + } > > + > > + break; > > + } > > + > > + case nir_instr_type_load_const: { > > + nir_load_const_instr *load = nir_instr_as_load_const(instr); > > + > > + /* For load_const instructions, it's a boolean exactly when it holds > > + * one of the values NIR_TRUE or NIR_FALSE. > > + * > > + * Since load_const instructions don't have any sources, we don't > > + * have to worry about resolving them. > > + */ > > + instr->pass_flags &= ~BRW_NIR_BOOLEAN_MASK; > > + if (load->value.u[0] == NIR_TRUE || load->value.u[0] == NIR_FALSE) { > > + instr->pass_flags |= BRW_NIR_BOOLEAN_NO_RESOLVE; > > + } else { > > + instr->pass_flags |= BRW_NIR_NON_BOOLEAN; > > + } > > + continue; > > + } > > + > > + default: > > + /* Everything else is an unknown non-boolean value and needs to > > + * have all sources resolved. > > + */ > > + instr->pass_flags = (instr->pass_flags & ~BRW_NIR_BOOLEAN_MASK) | > > + BRW_NIR_NON_BOOLEAN; > > + nir_foreach_src(instr, src_mark_needs_resolve, NULL); > > + continue; > > + } > > + } > > + > > + nir_if *following_if = nir_block_get_following_if(block); > > + if (following_if) > > + src_mark_needs_resolve(&following_if->condition, NULL); > > + > > + return true; > > +} > > + > > +static void > > +analyze_boolean_resolves_impl(nir_function_impl *impl) > > +{ > > + nir_foreach_block(impl, analyze_boolean_resolves_block, NULL); > > +} > > + > > +void > > +brw_nir_analyze_boolean_resolves(nir_shader *shader) > > +{ > > + nir_foreach_overload(shader, overload) > > + if (overload->impl) > > + analyze_boolean_resolves_impl(overload->impl); > > +} > > -- > > 2.3.3 > > > > _______________________________________________ > > mesa-dev mailing list > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a>