On 02/05/16 09:56, Iago Toral wrote: > On Mon, 2016-05-02 at 10:54 +0300, Pohjolainen, Topi wrote: >> On Mon, May 02, 2016 at 09:42:14AM +0200, Iago Toral 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. >> >> Ok. I would personally appreciate a comment clarifying this but as the >> current logic doesn't have one either, I'll leave it to you to add some text >> if you feel the same way. > > Sure, an extra comment won't hurt :) > > Iago >
As Iago said, we will add an extra comment here. Thanks, Sam _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev