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

Sorry, I'm not following what you mean with the last paragraph.  What
does this have to do with identifying DFs?

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

Is that really always the case?  Even on Vulkan?

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

Is the swizzle actually meant to be ignored or is that a bug of the
move_uniform_array_access_to_pull_constants() pass?  If it is meant to
be ignored why do we set a non-identity swizzle below for shift != 0 in
the indirect path a couple of lines below?

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

I'm not really that worried about consistency, just trying to understand
what exactly is going on in this patch in order to convince myself that
this is a complete fix.

>> > 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: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170424/163ccf4e/attachment-0001.sig>