[Mesa-dev] [PATCH v2 06/20] i965/fs: fix dst stride in IVB/BYT type conversions

[Samuel Iglesias Gonsálvez]

On Tue, 2017-01-17 at 13:26 -0800, Francisco Jerez wrote:
> Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:
> 
> > From: "Juan A. Suarez Romero" <jasuarez at igalia.com>
> > 
> > When converting a DF to F, we set dst stride to 2, to fulfil
> > alignment
> > restrictions.
> > 
> > But in IVB/BYT, this is not necessary, as each DF conversion
> > already
> > writes 2 F, the first one the real value, and the second one a 0.
> > That
> > is, IVB/BYT already set stride = 2 implicitly, so we must set it to
> > 1
> > explicitly to avoid ending up with stride = 4.
> > 
> > v2:
> > - Fix typo (Matt)
> > ---
> >  src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > index 45881e3ec95..487f2e90224 100644
> > --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
> > @@ -1629,6 +1629,16 @@ fs_generator::generate_code(const cfg_t
> > *cfg, int dispatch_width)
> >  		inst->src[i].type != BRW_REGISTER_TYPE_UD ||
> >  		!inst->src[i].negate);
> >        }
> > +      /* When converting from DF->F, we set destination's stride
> > as 2 as an
> > +       * alignment requirement. But in IVB/BYT, each DF implicitly
> > writes 2 F,
> > +       * being the first one the converted value. So we don't need
> > to
> > +       * explicitly set stride 2, but 1.
> > +       */
> > +      if (devinfo->gen == 7 && !devinfo->is_haswell &&
> > +          type_sz(inst->src[0].type) > type_sz(inst->dst.type)) {
> 
> This should be exec_type_size(inst) rather than
> type_sz(inst->src[0].type).
> 

Actually it is going to be the same as this is going to catch only DF
-> 32-bit data type conversions. But yeah, you are right.

> > +         assert(inst->dst.stride == 2 || inst->dst.stride == 1);
> > +         inst->dst.stride = 1;
> > +      }
> 
> This is modifying the IR, please don't.
> 

Right, I am going to do the change in brw_eu_emit.c inside the
brw_MOV() function that Matt added in other patch and also when
emitting the MOV indirect.

> Also I don't think the above has the same semantics as a destination
> region with stride 2...  AFAIUI IVB will just write garbage into the
> odd
> channels when the destination type is narrower than a DF which is
> really
> not what a strided move is supposed to do.  If that's the case it
> would
> probably be safer to add a new F64TO32 virtual opcode for type
> conversions and assert(inst->dst.stride == 1) here...
> 

I think adding a virtual opcode for this change is likely too much. I
will keep the aforementioned changes. However, I don't have a strong
opinion against it: if you prefer the virtual opcode, we can add it now
or even later as a follow-up patch.

Sam

> >        dst = brw_reg_from_fs_reg(compiler->devinfo, inst,
> >                                  &inst->dst, compressed);
> >  
> > -- 
> > 2.11.0
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170118/d9123bd5/attachment.sig>