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

[Matt Turner]

On Mon, May 2, 2016 at 12:42 AM, Iago Toral <itoral at igalia.com> wrote:
> 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.

FWIW, I suspect we'll have to change that line for 64-bit integer
support. Moving integers with via floats often yields bad results on
32-bit x86 (with the FPU) when the integer happens to be a NaN.