On Fri, Mar 20, 2015 at 3:41 PM, Matt Turner <matts...@gmail.com> wrote: > On Fri, Mar 20, 2015 at 2:23 PM, Jason Ekstrand <ja...@jlekstrand.net> 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 > > Ah, I misunderstood this comment on first read. You're saying that the > two things marked as NEEDS_RESOLVE will be resolved before the AND > consumes them, ... so the AND's destination will be marked with > NO_RESOLVE? > > That makes sense. > >> + * 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 <ja...@jlekstrand.net> >> + */ >> + >> +#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; > > I've got some patches to allow ir_unop_logic_not (and and, or, xor) to > operate per-component. That's not related to this comment though, > right?
Right. Those should get lowered by alu-to-scalar right now. I guess it might matter in vec4 and it should be safe for vectors. It just makes NIR -> FS a little more complicated. I can change that if you'd like. > As far as I can tell this looks good. It'd be cool if Connor could confirm. > > Reviewed-by: Matt Turner <matts...@gmail.com> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev