[Mesa-dev] [PATCH 04/13] i965: Mask out unused Align16 components in brw_untyped_atomic.

[Pohjolainen, Topi]

On Fri, Mar 06, 2015 at 02:56:24PM +0200, Francisco Jerez wrote:
> "Pohjolainen, Topi" <topi.pohjolainen at intel.com> writes:
> 
> > On Fri, Feb 27, 2015 at 05:34:47PM +0200, Francisco Jerez wrote:
> >> This is currently not a problem because the vec4 visitor happens to
> >> mask out unused components from the destination, but it might become
> >> an issue when we start using atomics without writeback message.  In
> >> any case it seems sensible to set it again here because the
> >> consequences of setting the wrong writemask (random graphics memory
> >> corruption) are difficult to debug and can easily go unnoticed.
> >
> > I started thinking if this should be an assertion here and should we force
> > the logic in the visitor to consider the writemask correctly instead? I don't
> > have a strong opinion, merely just wondering aloud.
> >
> 
> That would be rather inconvenient for my (not yet sent for review)
> ARB_shader_image_load_store intrinsic lowering code.  If we made it an
> assertion, say:
> 
> | emit(some_surface_opcode, vgrf(rlen), payload, surface, control);
> 
> or
> 
> | emit(some_surface_opcode, reg_null_ud(), payload, surface, control);
> 
> would fail with an assertion failure.  I need that to just work no
> matter what surface opcode is specified.
> 
> It would also be somewhat misleading, because these messages have the
> annoying property of clobbering destination register components even if
> they're masked out, so in Align16 they kind of always behave as if
> WRITEMASK_XYZW was specified as far as the destination region is
> concerned.

Just sent a reply with few nits. But otherwise I'm fine with the patch:

Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>