Samuel Iglesias Gonsálvez <sigles...@igalia.com> writes: > On Fri, 2017-04-21 at 10:23 -0700, Francisco Jerez wrote: >> Samuel Iglesias Gonsálvez <sigles...@igalia.com> writes: >> >> > On Thu, 2017-04-20 at 10:26 -0700, Francisco Jerez wrote: >> > > Samuel Iglesias Gonsálvez <sigles...@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 <sigles...@igalia.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? >> 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
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
