[Mesa-dev] [PATCH v2 06/20] i965/fs: fix dst stride in IVB/BYT type conversions

[Francisco Jerez]

Francisco Jerez <currojerez at riseup.net> writes:

> Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:
>
>> From: "Juan A. Suarez Romero" <jasuarez at igalia.com>
>>
>> When converting a DF to F, we set dst stride to 2, to fulfil alignment
>> restrictions.
>>
>> But in IVB/BYT, this is not necessary, as each DF conversion already
>> writes 2 F, the first one the real value, and the second one a 0. That
>> is, IVB/BYT already set stride = 2 implicitly, so we must set it to 1
>> explicitly to avoid ending up with stride = 4.
>>
>> v2:
>> - Fix typo (Matt)
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>> index 45881e3ec95..487f2e90224 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_generator.cpp
>> @@ -1629,6 +1629,16 @@ fs_generator::generate_code(const cfg_t *cfg, int dispatch_width)
>>  		inst->src[i].type != BRW_REGISTER_TYPE_UD ||
>>  		!inst->src[i].negate);
>>        }
>> +      /* When converting from DF->F, we set destination's stride as 2 as an
>> +       * alignment requirement. But in IVB/BYT, each DF implicitly writes 2 F,
>> +       * being the first one the converted value. So we don't need to
>> +       * explicitly set stride 2, but 1.
>> +       */
>> +      if (devinfo->gen == 7 && !devinfo->is_haswell &&
>> +          type_sz(inst->src[0].type) > type_sz(inst->dst.type)) {
>
> This should be exec_type_size(inst) rather than
> type_sz(inst->src[0].type).
>
>> +         assert(inst->dst.stride == 2 || inst->dst.stride == 1);
>> +         inst->dst.stride = 1;
>> +      }
>
> This is modifying the IR, please don't.
>
> Also I don't think the above has the same semantics as a destination
> region with stride 2...  AFAIUI IVB will just write garbage into the odd
> channels when the destination type is narrower than a DF which is really
> not what a strided move is supposed to do.  If that's the case it would
> probably be safer to add a new F64TO32 virtual opcode for type
> conversions and assert(inst->dst.stride == 1) here...
>

It seems like my intuition matches the simulator's behavior,
unfortunately...

>>        dst = brw_reg_from_fs_reg(compiler->devinfo, inst,
>>                                  &inst->dst, compressed);
>>  
>> -- 
>> 2.11.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://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: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170117/69e51319/attachment.sig>