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

[Francisco Jerez]

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

>> ---
>>  src/mesa/drivers/dri/i965/brw_eu_emit.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>> 
>> diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c b/src/mesa/drivers/dri/i965/brw_eu_emit.c
>> index 2b1d6ff..0b655d4 100644
>> --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
>> +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
>> @@ -2799,16 +2799,25 @@ brw_untyped_atomic(struct brw_compile *p,
>>                     bool response_expected)
>>  {
>>     const struct brw_context *brw = p->brw;
>> +   const bool align1 = (brw_inst_access_mode(brw, p->current) == BRW_ALIGN_1);
>> +   /* Mask out unused components -- This is especially important in Align16
>> +    * mode on generations that don't have native support for SIMD4x2 atomics,
>> +    * because unused but enabled components will cause the dataport to perform
>> +    * additional atomic operations on the addresses that happen to be in the
>> +    * uninitialized Y, Z and W coordinates of the payload.
>> +    */
>> +   const unsigned mask = (align1 ? WRITEMASK_XYZW : WRITEMASK_X);
>>     brw_inst *insn = brw_next_insn(p, BRW_OPCODE_SEND);
>>  
>> -   brw_set_dest(p, insn, retype(dest, BRW_REGISTER_TYPE_UD));
>> +   brw_set_dest(p, insn, retype(brw_writemask(dest, mask),
>> +                                BRW_REGISTER_TYPE_UD));
>>     brw_set_src0(p, insn, retype(payload, BRW_REGISTER_TYPE_UD));
>>     brw_set_src1(p, insn, brw_imm_d(0));
>>     brw_set_dp_untyped_atomic_message(
>>        p, insn, atomic_op, bind_table_index, msg_length,
>>        brw_surface_payload_size(p, response_expected,
>>                                 brw->gen >= 8 || brw->is_haswell, true),
>> -      brw_inst_access_mode(brw, insn) == BRW_ALIGN_1);
>> +      align1);
>>  }
>>  
>>  static void
>> -- 
>> 2.1.3
>> 
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150306/92d2b47b/attachment.sig>