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

[Francisco Jerez]

Samuel Iglesias Gonsálvez  writes:

> On Mon, 2017-04-24 at 11:22 -0700, Francisco Jerez wrote:
>> Samuel Iglesias Gonsálvez  writes:
>> 
>> > On Fri, 2017-04-21 at 10:23 -0700, Francisco Jerez wrote:
>> > > Samuel Iglesias Gonsálvez  writes:
>> > > 
>> > > > On Thu, 2017-04-20 at 10:26 -0700, Francisco Jerez wrote:
>> > > > > Samuel Iglesias Gonsálvez  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 > > > > > > com>
>> > > > > > Cc: curroje...@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?
>> 
>
> By the register type, I know it is a DF, but I need then to know how
> many components are used in order to know if they are dvec3 or dvec4,
> so I push them first (so they are aligned to dvec4), or just dvec2 or a
> double scalar, so I push them later. This is whay I meant when I said
> identifying DF, sorry for the misunderstanding.
>

Sounds like the problem is not specific to DFs in that case?

>> > > 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?
>
> For GL, yes.
>
>>   Even on Vulkan?
>> 
>
> Good question! Looking at the code, Vulkan driver doesn't support pull
> constants other than UBOs so everything has to be pushed regardless.
>
> In the push constant buffer code, it marks every register touched by
> MOV INDIRECT as fully used, ignoring its swizzle because it is just
> interested on the number of read vec4 slots and consider we fully read
> them, which is also mentioned in the quote I pasted in a previous
> email:
>
> /* We just mark every register touched by a MOV_INDIRECT as being
>  * fully used.  

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

[Samuel Iglesias Gonsálvez]

On Mon, 2017-04-24 at 11:22 -0700, Francisco Jerez wrote:
> Samuel Iglesias Gonsálvez  writes:
> 
> > On Fri, 2017-04-21 at 10:23 -0700, Francisco Jerez wrote:
> > > Samuel Iglesias Gonsálvez  writes:
> > > 
> > > > On Thu, 2017-04-20 at 10:26 -0700, Francisco Jerez wrote:
> > > > > Samuel Iglesias Gonsálvez  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  > > > > > com>
> > > > > > Cc: curroje...@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?
> 

By the register type, I know it is a DF, but I need then to know how
many components are used in order to know if they are dvec3 or dvec4,
so I push them first (so they are aligned to dvec4), or just dvec2 or a
double scalar, so I push them later. This is whay I meant when I said
identifying DF, sorry for the misunderstanding.

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

For GL, yes.

>   Even on Vulkan?
> 

Good question! Looking at the code, Vulkan driver doesn't support pull
constants other than UBOs so everything has to be pushed regardless.

In the push constant buffer code, it marks every register touched by
MOV INDIRECT as fully used, ignoring its swizzle because it is just
interested on the number of read vec4 slots and consider we fully read
them, which is also mentioned in the quote I pasted in a previous
email:

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

When the swizzle is used then? The swizzle is taking into account in
the generator to add it as an offset when the indirect offset is not an
immediate, or add it to the 

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

[Francisco Jerez]

Samuel Iglesias Gonsálvez  writes:

> On Fri, 2017-04-21 at 10:23 -0700, Francisco Jerez wrote:
>> Samuel Iglesias Gonsálvez  writes:
>> 
>> > On Thu, 2017-04-20 at 10:26 -0700, Francisco Jerez wrote:
>> > > Samuel Iglesias Gonsálvez  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 
>> > > > Cc: curroje...@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

Re: [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  writes:
> 
> > On Thu, 2017-04-20 at 10:26 -0700, Francisco Jerez wrote:
> > > Samuel Iglesias Gonsálvez  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 
> > > > Cc: curroje...@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

signature.asc
Description: This is a digitally signed message part
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Francisco Jerez]

Samuel Iglesias Gonsálvez  writes:

> On Thu, 2017-04-20 at 10:26 -0700, Francisco Jerez wrote:
>> Samuel Iglesias Gonsálvez  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 
>> > Cc: curroje...@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


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Samuel Iglesias Gonsálvez]

On Thu, 2017-04-20 at 10:26 -0700, Francisco Jerez wrote:
> Samuel Iglesias Gonsálvez  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 
> > Cc: curroje...@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).

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

Sam

> >   src.swizzle += BRW_SWIZZLE4(shift, shift, shift, shift);
> >  
> >   emit(MOV(dest, src));
> > -- 
> > 2.11.0

signature.asc
Description: This is a digitally signed message part
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Francisco Jerez]

Samuel Iglesias Gonsálvez  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 
> Cc: curroje...@riseup.net

Don't you need to CC mesa-stable here and in the next patch?

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

>   src.swizzle += BRW_SWIZZLE4(shift, shift, shift, shift);
>  
>   emit(MOV(dest, src));
> -- 
> 2.11.0


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev