On Mon, May 2, 2016 at 12:42 AM, Iago Toral <ito...@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 <ito...@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. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev