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

[Francisco Jerez]

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?),
how come it is not a problem for indirect moves?

> 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.

> 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: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170421/48eccd50/attachment.sig>