On Fri, Mar 20, 2015 at 1:26 PM, Matt Turner <matts...@gmail.com> wrote: > On Fri, Mar 20, 2015 at 11:24 AM, Jason Ekstrand <ja...@jlekstrand.net> wrote: >> v2: Fix the spelling of analyze and re-arrange code for better readability >> as per Connor's comments. >> --- >> src/mesa/drivers/dri/i965/Makefile.sources | 2 + >> src/mesa/drivers/dri/i965/brw_nir.h | 45 ++++ >> .../dri/i965/brw_nir_analyze_boolean_resolves.c | 228 >> +++++++++++++++++++++ >> 3 files changed, 275 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..4a81647 >> --- /dev/null >> +++ b/src/mesa/drivers/dri/i965/brw_nir.h >> @@ -0,0 +1,45 @@ >> +/* >> + * 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 > > I think we decided that we still need actual include guards. > >> + >> +#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, > > Meaning is obvious. > >> + BRW_NIR_BOOLEAN_NEEDS_RESOLVE = 0x1, > > We decided this instruction's destination needs to be resolved? > >> + BRW_NIR_BOOLEAN_UNRESOLVED = 0x2, > > This is different from BRW_NIR_BOOLEAN_NEEDS_RESOLVE in > > cmp res1 ... > cmp res2 ... > and res3 res2 res1 > > in that res1 and res2 are BRW_NIR_BOOLEAN_UNRESOLVED while res3 is > BRW_NIR_BOOLEAN_NEEDS_RESOLVE? > >> + BRW_NIR_BOOLEAN_NO_RESOLVE = 0x3, > > I'm not sure what this one means.
I've added piles of comments. Will send a v2. > We're doing bit operations below on these enum values, so it's > surprising to see that > > BRW_NIR_BOOLEAN_NEEDS_RESOLVE | BRW_NIR_BOOLEAN_UNRESOLVED == > BRW_NIR_BOOLEAN_NO_RESOLVE > > Is that intentional? No. Any bitwise operations should purely be for the purpose of masking off the bottom 2 bits of the pass_flags. That way we can safely use the other 6 bits of pass_flags for something else if we want. Any other use of bitwise operations is a bug. > All in all, some comments explaining what these enums mean would help > the reader significantly. > >> + 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..b19d80b >> --- /dev/null >> +++ b/src/mesa/drivers/dri/i965/brw_nir_analyze_boolean_resolves.c >> @@ -0,0 +1,228 @@ >> +/* >> + * 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 > > It doesn't stand for anything, so we just write Gen. Yup >> + * will have the booleans portion of the instr->pass_flags field set to >> + * BRW_NIR_BOOLEAN_NEEDS_RESOLVE. >> + */ >> + >> +static uint8_t >> +get_resolve_state_for_src(nir_alu_instr *alu, unsigned src_idx) >> +{ >> + nir_instr *src_instr; >> + if (alu->src[src_idx].src.is_ssa) { >> + src_instr = alu->src[src_idx].src.ssa->parent_instr; >> + } else { >> + src_instr = alu->src[src_idx].src.reg.reg->parent_instr; >> + } >> + >> + if (src_instr) { >> + uint8_t state = src_instr->pass_flags & BRW_NIR_BOOLEAN_MASK; >> + >> + /* If the source instruction nees resolve then, from the perspective > > typo: needs > > I had a hard time parsing this sentence until I mentally moved the > comma from after 'then' to before it. > >> + * of the user, it's a true boolean. >> + */ >> + if (state == BRW_NIR_BOOLEAN_NEEDS_RESOLVE) >> + state = BRW_NIR_BOOLEAN_NO_RESOLVE; >> + return state; >> + } else { >> + return BRW_NIR_NON_BOOLEAN; >> + } >> +} >> + >> +static bool >> +src_mark_needs_resolve(nir_src *src, void *void_state) >> +{ >> + if (src->is_ssa) >> + return true; >> + >> + if (src->reg.reg->parent_instr == NULL) >> + return true; >> + >> + nir_instr *src_instr = src->reg.reg->parent_instr; >> + >> + uint8_t bool_flags = src_instr->pass_flags & BRW_NIR_BOOLEAN_MASK; >> + if (bool_flags == 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) { >> + /* Clear the boolean state */ >> + instr->pass_flags &= ~BRW_NIR_BOOLEAN_MASK; >> + >> + switch (instr->type) { >> + case nir_instr_type_alu: { >> + uint8_t bool_status; > > Reading uses of this variable is difficult... "bool status equals ..." > > Change it to "resolve_status"? Yeah, I've changed the entire thing to consistently use "resolve status" --Jason _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev