[Mesa-dev] [PATCH 03/19] i965/fs: Combine fs_inst constructors using default parameters.

[Kenneth Graunke]

On 04/18/2014 11:56 AM, Matt Turner wrote:
> Wouldn't it be nice if case labels could be non-constant expressions.
> ---
>  src/mesa/drivers/dri/i965/brw_fs.cpp | 77 +++++++++++++-----------------------
>  src/mesa/drivers/dri/i965/brw_fs.h   | 10 ++---
>  2 files changed, 31 insertions(+), 56 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index b0d6e4e..bb2d290 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -74,69 +74,46 @@ fs_inst::fs_inst()
>     this->opcode = BRW_OPCODE_NOP;
>  }
>  
> -fs_inst::fs_inst(enum opcode opcode)
> +fs_inst::fs_inst(enum opcode opcode, const fs_reg &dst,
> +                 const fs_reg &src0, const fs_reg &src1, const fs_reg &src2)
>  {
> -   init();
> -   this->opcode = opcode;
> -}
> +   if (&dst == &reg_undef)
> +      assert(&src0 == &reg_undef);
> +   if (&src0 == &reg_undef)
> +      assert(&src1 == &reg_undef);
> +   if (&src1 == &reg_undef)
> +      assert(&src2 == &reg_undef);
>  
> -fs_inst::fs_inst(enum opcode opcode, fs_reg dst)
> -{
>     init();
> -   this->opcode = opcode;
> -   this->dst = dst;
>  
> -   if (dst.file == GRF)
> -      assert(dst.reg_offset >= 0);
> -}
> +   if (&src2 != &reg_undef) {
> +      goto src2;
> +   } else if (&src1 != &reg_undef) {
> +      goto src1;
> +   } else if (&src0 != &reg_undef) {
> +      goto src0;
> +   } else if (&dst != &reg_undef) {
> +      goto dst;
> +   }
>  
> -fs_inst::fs_inst(enum opcode opcode, fs_reg dst, fs_reg src0)
> -{
> -   init();
> -   this->opcode = opcode;
> -   this->dst = dst;
> +src2:
> +   this->src[2] = src2;
> +   if (src[2].file == GRF)
> +      assert(src[2].reg_offset >= 0);
> +src1:
> +   this->src[1] = src1;
> +   if (src[1].file == GRF)
> +      assert(src[1].reg_offset >= 0);
> +src0:
>     this->src[0] = src0;
> -
> -   if (dst.file == GRF)
> -      assert(dst.reg_offset >= 0);
>     if (src[0].file == GRF)
>        assert(src[0].reg_offset >= 0);
> -}
> -
> -fs_inst::fs_inst(enum opcode opcode, fs_reg dst, fs_reg src0, fs_reg src1)
> -{
> -   init();
> -   this->opcode = opcode;
> +dst:
>     this->dst = dst;
> -   this->src[0] = src0;
> -   this->src[1] = src1;
> -
>     if (dst.file == GRF)
>        assert(dst.reg_offset >= 0);
> -   if (src[0].file == GRF)
> -      assert(src[0].reg_offset >= 0);
> -   if (src[1].file == GRF)
> -      assert(src[1].reg_offset >= 0);
> -}
>  
> -fs_inst::fs_inst(enum opcode opcode, fs_reg dst,
> -		 fs_reg src0, fs_reg src1, fs_reg src2)
> -{
> -   init();
>     this->opcode = opcode;
> -   this->dst = dst;
> -   this->src[0] = src0;
> -   this->src[1] = src1;
> -   this->src[2] = src2;
> -
> -   if (dst.file == GRF)
> -      assert(dst.reg_offset >= 0);
> -   if (src[0].file == GRF)
> -      assert(src[0].reg_offset >= 0);
> -   if (src[1].file == GRF)
> -      assert(src[1].reg_offset >= 0);
> -   if (src[2].file == GRF)
> -      assert(src[2].reg_offset >= 0);
>  }
>  
>  fs_inst::fs_inst(const fs_inst &that)
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
> index 8af4520..8e2af4f 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -192,12 +192,10 @@ public:
>     void init();
>  
>     fs_inst();
> -   fs_inst(enum opcode opcode);
> -   fs_inst(enum opcode opcode, fs_reg dst);
> -   fs_inst(enum opcode opcode, fs_reg dst, fs_reg src0);
> -   fs_inst(enum opcode opcode, fs_reg dst, fs_reg src0, fs_reg src1);
> -   fs_inst(enum opcode opcode, fs_reg dst,
> -           fs_reg src0, fs_reg src1,fs_reg src2);
> +   fs_inst(enum opcode opcode, const fs_reg &dst = reg_undef,
> +           const fs_reg &src0 = reg_undef,
> +           const fs_reg &src1 = reg_undef,
> +           const fs_reg &src2 = reg_undef);
>     fs_inst(const fs_inst &that);
>  
>     bool equals(fs_inst *inst) const;
> 

I really don't like this code...lots if if, goto, and reliance on
pointer comparisons with global variables.

What about instead making init() look like your array constructor, and
building everything on top of that?  i.e.

void
fs_inst::init(enum opcode opcode, const fs_reg &dst, int sources = 1,
fs_reg *src = NULL)
{
   memset(this, 0, sizeof(*this));

   this->sources = sources;
   if (src) {
      this->src = src;
   } else {
      this->src = ralloc_array(this, fs_reg, sources);
      for (int i = 0; i < sources; i++)
         this->src[i] = reg_undef;
   }

   this->conditional_mod = BRW_CONDITIONAL_NONE;

   this->dst = dst;

   /* This will be the case for almost all instructions. */
   this->regs_written = 1;

   this->writes_accumulator = false;
}

fs_inst::fs_inst()
{
   init(BRW_OPCODE_NOP, reg_undef);
}

fs_inst::fs_inst(enum opcode opcode, const fs_reg &dst)
{
   init(opcode, dst);
}

fs_inst::fs_inst(enum opcode opcode, const fs_reg &dst,
                 const fs_reg &src0)
{
   fs_reg *src = ralloc_array(this, fs_reg, 1);
   src[0] = src0;
   init(opcode, dst, 1, src);
}

fs_inst::fs_inst(enum opcode opcode, const fs_reg &dst,
                 const fs_reg &src0, const fs_reg &src1)
{
   fs_reg *src = ralloc_array(this, fs_reg, 2);
   src[0] = src0;
   src[1] = src1;
   init(opcode, dst, 2, src);
}

fs_inst::fs_inst(enum opcode opcode, const fs_reg &dst,
                 const fs_reg &src0, const fs_reg &src1)
{
   fs_reg *src = ralloc_array(this, fs_reg, 3);
   src[0] = src0;
   src[1] = src1;
   src[2] = src2;
   init(opcode, dst, 3, src);
}

fs_inst::fs_inst(enum opcode opcode, const fs_reg &dst,
                 fs_reg src[], int sources)
{
   init(opcode, dst, sources, src);
   this->regs_written = sources;
}

This also has the benefit that all fs_inst constructors, including your
new array one, continue to go through init().  Without that, I'm afraid
we'll forget to initialize some field in the array constructor case.

--Ken

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140423/46f4c418/attachment.sig>