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

[Pohjolainen, Topi]

On Thu, Feb 20, 2014 at 01:41:29PM -0800, 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 77b9f57..77cdfa2 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -66,69 +66,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);

I know this is not enforced originally either but shouldn't this apply also?

   if (&src0 == &reg_undef)
      assert(&src1 == &reg_undef);
   else
      assert(&dst != &reg_undef);

   if (&src1 == &reg_undef)
      assert(&src2 == &reg_undef);
   else
      assert(&src0 != &reg_undef);

   if (&src2 != &reg_undef)
      assert(&src1 != &reg_undef);

I also tried to write the "switch-case" in a different way but at least I
cannot come up with anything better than yours. With or without the stuff
above:

Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com>

>  
> -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 77df64b..f73f835 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -176,12 +176,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);
> -- 
> 1.8.3.2
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev