[Mesa-dev] [PATCH v3 5/7] i965: Add a NIR analysis pass for determining when a boolean resolve is needed

Jason Ekstrand jason at jlekstrand.net
Sun Mar 22 20:55:44 PDT 2015


On Mar 22, 2015 8:48 PM, "Connor Abbott" <cwabbott0 at gmail.com> wrote:
>
> On Fri, Mar 20, 2015 at 5:23 PM, Jason Ekstrand <jason at 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
> > +    * 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 <jason at 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;
>
> 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 <cwabbott0 at gmail.com>
>
> > +
> > +            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
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150322/5eaca073/attachment-0001.html>


More information about the mesa-dev mailing list