[Mesa-dev] [PATCH 1/2] i965/vec4: set swizzle when loading an uniform

[Samuel Iglesias Gonsálvez]

On Fri, 2017-04-21 at 10:23 -0700, Francisco Jerez wrote:
> Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:
> 
> > On Thu, 2017-04-20 at 10:26 -0700, Francisco Jerez wrote:
> > > Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:
> > > 
> > > > It was setting XYWZ swizzle to all uniforms, no matter if they
> > > > were
> > > > a vector or not.
> > > > 
> > > > Signed-off-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
> > > > Cc: currojerez at riseup.net
> > > 
> > > Don't you need to CC mesa-stable here and in the next patch?
> > > 
> > 
> > I considered it but I has doubts about which tag use "17.1.0-rc1"
> > or
> > just "17.1.0" or whatever. So my plan is to notify Emil once they
> > are
> > merged (and add Cc to stable in the commit log before pushing it to
> > master).
> > 
> > If you are more comfortable with Cc mesa-stable, I will do it next
> > time
> > (or if I need to send v2 of this series).
> > 
> 
> I believe Emil will notice them even if you don't put a version
> tag.  I
> don't really care how you nominate it for stable as long as it hits
> the
> 17.1 branch before the end of the release cycle.  ;)
> 
> > > > ---
> > > >  src/intel/compiler/brw_vec4_nir.cpp | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/src/intel/compiler/brw_vec4_nir.cpp
> > > > b/src/intel/compiler/brw_vec4_nir.cpp
> > > > index a82d52088a8..5f4488c7e86 100644
> > > > --- a/src/intel/compiler/brw_vec4_nir.cpp
> > > > +++ b/src/intel/compiler/brw_vec4_nir.cpp
> > > > @@ -863,6 +863,7 @@
> > > > vec4_visitor::nir_emit_intrinsic(nir_intrinsic_instr *instr)
> > > >           unsigned offset = const_offset->u32[0] + shift * 4;
> > > >           src.offset = ROUND_DOWN_TO(offset, 16);
> > > >           shift = (offset % 16) / 4;
> > > > +         src.swizzle = brw_swizzle_for_size(instr-
> > > > > num_components);
> > > 
> > > What about the indirect case a few lines below?  Isn't the
> > > swizzle
> > > passed
> > > to the mov indirect instruction still bogus?
> > > 
> > 
> > This is different. It is expecting to have a swizzle of XYZW
> > because
> > MOV_INDIRECT will copy all the contents. See assert in
> > move_uniform_array_access_to_pull_constants()
> 
> I believe the ultimate problem here is that the MOV_INDIRECT gets a
> writemask of XYZW even if you're reading a scalar, so setting the
> minimal swizzle will lead to a situation in which the resulting
> swizzle
> is not the identity so you will run into trouble to turn it into
> scratch
> access.  That brings me to the following question which I don't think
> I
> can answer by looking at this patch alone: If having an XYZW swizzle
> is
> a problem for direct moves (I assume this patch is fixing
> something?),

The bug is to properly identify DFs and dvecs, instead of considering
all of them as dvec4 when aligning them for the push constant buffer,
which is done in next patch.

> how come it is not a problem for indirect moves?
> 

It is not a problem because the uniforms under indirect moves are moved
to pull constant buffer in
move_uniform_array_access_to_pull_constants().

Those DF pull constants are read with 2 messages, so they don't need to
be dvec4-aligned like in the case of direct moves on the push constant
buffer, and the swizzle is actually ignored when loading it to pull
constant buffer.

However, if you prefer to keep consistency between both cases, I can
apply the shuffle globally and remove the NOOP assert in
move_uniform_array_access_to_pull_constants(). This change doesn't add
any regression on piglit.

What do you think?

> > and the comment in pack_uniform_registers():
> > 
> > /* We just mark every register touched by a MOV_INDIRECT as being
> >  * fully used.  This ensures that it doesn't broken up piecewise by
> >  * the next part of our packing algorithm.
> >  */
> > 
> 
> Not sure why this is relevant to the discussion?  The same will be
> the
> case if you set the same swizzle as for the direct move.
> 

Right, I was wrong. Sorry for the noise,

Sam

> > Sam
> > 
> > > >           src.swizzle += BRW_SWIZZLE4(shift, shift, shift,
> > > > shift);
> > > >  
> > > >           emit(MOV(dest, src));
> > > > -- 
> > > > 2.11.0
-------------- 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/20170424/094e9e6a/attachment-0001.sig>