[Mesa-dev] [PATCH 18/59] i965: fix brw_saturate_immediate() for doubles

[Iago Toral]

On Mon, 2016-05-02 at 10:08 +0300, Pohjolainen, Topi wrote:
> On Mon, May 02, 2016 at 10:02:50AM +0300, Pohjolainen, Topi wrote:
> > On Fri, Apr 29, 2016 at 01:29:15PM +0200, Samuel Iglesias Gons?lvez wrote:
> > > From: Iago Toral Quiroga <itoral at igalia.com>
> > > 
> > > ---
> > >  src/mesa/drivers/dri/i965/brw_shader.cpp | 28 ++++++++++++++++++++++------
> > >  1 file changed, 22 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp b/src/mesa/drivers/dri/i965/brw_shader.cpp
> > > index d40937b..a063b88 100644
> > > --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
> > > +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
> > > @@ -476,7 +476,14 @@ brw_saturate_immediate(enum brw_reg_type type, struct brw_reg *reg)
> > >        unsigned ud;
> > >        int d;
> > >        float f;
> > > -   } imm = { reg->ud }, sat_imm = { 0 };
> > > +      double df;
> > > +   } imm, sat_imm = { 0 };
> > > +
> > > +   unsigned size = type_sz(type);
> > 
> > Could be 'const'.
> > 
> > > +   if (size < 8)
> 
> Thinking a little further, is there a reason we don't write directly:
> 
>         if (type == BRW_REGISTER_TYPE_DF)
>            imm.df = reg->df;
>         else
>            imm.ud = reg->ud;

Because in the future we might want to support 64-bit integers too
(BRW_REGISTER_TYPE_Q I think?) and then we would need to change this
code again. The original implementation would do the right thing in that
case without changes.

> > > +      imm.ud = reg->ud;
> > > +   else
> > > +      imm.df = reg->df;
> > >  
> > >     switch (type) {
> > >     case BRW_REGISTER_TYPE_UD:
> > > @@ -490,6 +497,9 @@ brw_saturate_immediate(enum brw_reg_type type, struct brw_reg *reg)
> > >     case BRW_REGISTER_TYPE_F:
> > >        sat_imm.f = CLAMP(imm.f, 0.0f, 1.0f);
> > >        break;
> > > +   case BRW_REGISTER_TYPE_DF:
> > > +      sat_imm.df = CLAMP(imm.df, 0.0, 1.0);
> > > +      break;
> > >     case BRW_REGISTER_TYPE_UB:
> > >     case BRW_REGISTER_TYPE_B:
> > >        unreachable("no UB/B immediates");
> > > @@ -497,14 +507,20 @@ brw_saturate_immediate(enum brw_reg_type type, struct brw_reg *reg)
> > >     case BRW_REGISTER_TYPE_UV:
> > >     case BRW_REGISTER_TYPE_VF:
> > >        unreachable("unimplemented: saturate vector immediate");
> > > -   case BRW_REGISTER_TYPE_DF:
> > >     case BRW_REGISTER_TYPE_HF:
> > > -      unreachable("unimplemented: saturate DF/HF immediate");
> > > +      unreachable("unimplemented: saturate HF immediate");
> > >     }
> > >  
> > > -   if (imm.ud != sat_imm.ud) {
> > > -      reg->ud = sat_imm.ud;
> > > -      return true;
> > > +   if (size < 8) {
> 
> Same here:
> 
>         if (type != BRW_REGISTER_TYPE_DF)
> 
> > > +      if (imm.ud != sat_imm.ud) {
> > > +         reg->ud = sat_imm.ud;
> > > +         return true;
> > > +      }
> > > +   } else {
> > > +      if (imm.df != sat_imm.df) {
> > > +         reg->df = sat_imm.df;
> > > +         return true;
> > > +      }
> > >     }
> > >     return false;
> > >  }
> > > -- 
> > > 2.5.0
> > > 
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev