[Mesa-dev] [PATCH 01/24] i965/fs: Define methods to calculate the flag subset read or written by an fs_inst.

[Jason Ekstrand]

On Thu, May 26, 2016 at 8:46 PM, Francisco Jerez <currojerez at riseup.net>
wrote:

> ---
>  src/mesa/drivers/dri/i965/brw_fs.cpp  | 54
> +++++++++++++++++++++++++++++------
>  src/mesa/drivers/dri/i965/brw_ir_fs.h | 25 ++++++++++++++--
>  2 files changed, 68 insertions(+), 11 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
> b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 32fa816..c95da29 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -907,19 +907,55 @@ fs_inst::regs_read(int arg) const
>     return 0;
>  }
>
> -bool
> -fs_inst::reads_flag() const
> +namespace {
> +   /* Return the subset of flag registers that an instruction could
> +    * potentially read or write based on the execution controls and flag
> +    * subregister number of the instruction.
> +    */
> +   uint32_t
> +   flag_mask(const fs_inst *inst)
> +   {
> +      const unsigned start = inst->flag_subreg * 16 + inst->group;
> +      const unsigned end = start + inst->exec_size;
> +      return ((1 << DIV_ROUND_UP(end, 8)) - 1) & ~((1 << (start / 8)) -
> 1);
> +   }
> +}
> +
> +uint32_t
>

Should these return a uint8_t or maybe just unsigned instead?  Explicitly
returning a uint32_t makes it look like it's the 32 possible 1-bit flag
values where, in reality, it's the 4 possible flag subregisters.


> +fs_inst::flags_read(const brw_device_info *devinfo) const
>  {
> -   return predicate;
> +   /* XXX - This doesn't consider explicit uses of the flag register as
> source
> +    *       region.
> +    */
> +   if (predicate == BRW_PREDICATE_ALIGN1_ANYV ||
> +       predicate == BRW_PREDICATE_ALIGN1_ALLV) {
> +      /* The vertical predication modes combine corresponding bits from
> +       * f0.0 and f1.0 on Gen7+, and f0.0 and f0.1 on older hardware.
> +       */
> +      const unsigned shift = devinfo->gen >= 7 ? 4 : 2;
> +      return flag_mask(this) << shift | flag_mask(this);
> +
> +   } else if (predicate) {
> +      return flag_mask(this);
> +
>

Do we need this blank line or the one above?  I'd delete them personally.


> +   } else {
> +      return 0;
> +   }
>  }
>
> -bool
> -fs_inst::writes_flag() const
> +uint32_t
> +fs_inst::flags_written() const
>  {
> -   return (conditional_mod && (opcode != BRW_OPCODE_SEL &&
> -                               opcode != BRW_OPCODE_IF &&
> -                               opcode != BRW_OPCODE_WHILE)) ||
> -          opcode == FS_OPCODE_MOV_DISPATCH_TO_FLAGS;
> +   /* XXX - This doesn't consider explicit uses of the flag register as
> +    *       destination region.
> +    */
> +   if ((conditional_mod && (opcode != BRW_OPCODE_SEL &&
> +                            opcode != BRW_OPCODE_IF &&
> +                            opcode != BRW_OPCODE_WHILE)) ||
> +       opcode == FS_OPCODE_MOV_DISPATCH_TO_FLAGS)
> +      return flag_mask(this);
> +   else
> +      return 0;
>

It would be nice if this if had braces.  I know it's just a one-liner when
it comes to the body of the if/else but the condition is complex enough
it's kind of hard to see where the condition ends and the body begins.


>  }
>
>  /**
> diff --git a/src/mesa/drivers/dri/i965/brw_ir_fs.h
> b/src/mesa/drivers/dri/i965/brw_ir_fs.h
> index d7ca406..1bac3bb 100644
> --- a/src/mesa/drivers/dri/i965/brw_ir_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_ir_fs.h
> @@ -274,8 +274,29 @@ public:
>     bool has_side_effects() const;
>     bool has_source_and_destination_hazard() const;
>
> -   bool reads_flag() const;
> -   bool writes_flag() const;
> +   /**
> +    * Return the subset of flag registers read by the instruction as a
> bitset
> +    * with byte granularity.
> +    */
> +   uint32_t flags_read(const brw_device_info *devinfo) const;
> +
> +   /**
> +    * Return the subset of flag registers updated by the instruction
> (either
> +    * partially or fully) as a bitset with byte granularity.
> +    */
> +   uint32_t flags_written() const;
> +
> +   bool reads_flag() const
> +   {
> +      /* XXX - Will get rid of this hack shortly. */
> +      const brw_device_info devinfo = {};
> +      return flags_read(&devinfo);
> +   }
> +
> +   bool writes_flag() const
> +   {
> +      return flags_written();
> +   }
>
>     fs_reg dst;
>     fs_reg *src;
> --
> 2.7.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160527/2091d175/attachment.html>