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

[Iago Toral]

On Mon, 2016-05-02 at 10:34 +0300, Pohjolainen, Topi wrote:
> On Mon, May 02, 2016 at 09:22:49AM +0200, Iago Toral wrote:
> > 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.
> 
> I was thinking about this as this idiom works elsewhere just fine. But here
> wouldn't 64-bit integers have size >= 8 and therefore use:
> 
>                imm.df = reg->df;
> 
> Or is this the intention as it copies 64-bits regardless of the type?

Exactly, that's what we want to do here. Basically, we want to either do
a 32-bit or 64-bit data copy, the type is otherwise irrelevant and doing
it this way makes it so that we don't have to patch the code again in
the future if we support 64-bit integers. Also, it is consistent with
the way in which we were already handling 32-bit types where we used ud
for all of them.

Iago