[Mesa-dev] [PATCH v3 21/24] i965: Use correct VertStride on align16 instructions.

[Samuel Iglesias Gonsálvez]

On 04/03/17 01:44, Francisco Jerez wrote:
> Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:
> 
>> From: Matt Turner <mattst88 at gmail.com>
>>
>> In commit c35fa7a, we changed the "width" of DF source registers to 2,
>> which is conceptually fine. Unfortunately a VertStride of 2 is not
>> allowed by align16 instructions on IVB/BYT, and the regular VertStride
>> of 4 works fine in any case.
>>
>> See generated_tests/spec/arb_gpu_shader_fp64/execution/built-in-functions/vs-round-double.shader_test
>> for example:
>>
>> cmp.ge.f0(8)    g18<1>DF        g1<0>.xyxyDF    -g8<2>DF        { align16 1Q };
>>         ERROR: In Align16 mode, only VertStride of 0 or 4 is allowed
>> cmp.ge.f0(8)    g19<1>DF        g1<0>.xyxyDF    -g9<2>DF        { align16 2N };
>>         ERROR: In Align16 mode, only VertStride of 0 or 4 is allowed
>>
>> v2:
>> - Add spec quote (Curro).
>> - Change the condition to only BRW_VERTICAL_STRIDE_2 (Curro)
>>
>> Reviewed-by: Samuel Iglesias Gonsálvez <siglesias at igalia.com>
>> ---
>>  src/mesa/drivers/dri/i965/brw_eu_emit.c | 44 +++++++++++++++++++++++++--------
>>  1 file changed, 34 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c b/src/mesa/drivers/dri/i965/brw_eu_emit.c
>> index 03aaa760163..d221405db4d 100644
>> --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
>> +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
>> @@ -512,13 +512,25 @@ brw_set_src0(struct brw_codegen *p, brw_inst *inst, struct brw_reg reg)
>>           brw_inst_set_src0_da16_swiz_w(devinfo, inst,
>>              BRW_GET_SWZ(reg.swizzle, BRW_CHANNEL_W));
>>  
>> -	 /* This is an oddity of the fact we're using the same
>> -	  * descriptions for registers in align_16 as align_1:
>> -	  */
>> -	 if (reg.vstride == BRW_VERTICAL_STRIDE_8)
>> +         if (reg.vstride == BRW_VERTICAL_STRIDE_8) {
>> +            /* This is an oddity of the fact we're using the same
>> +             * descriptions for registers in align_16 as align_1:
>> +             */
>> +            brw_inst_set_src0_vstride(devinfo, inst, BRW_VERTICAL_STRIDE_4);
>> +         } else if (devinfo->gen == 7 && !devinfo->is_haswell &&
>> +                    reg.type == BRW_REGISTER_TYPE_DF &&
>> +                    reg.vstride == BRW_VERTICAL_STRIDE_2) {
>> +            /* From HSW PRM:
> 
> This workaround is IVB-specific so the HSW PRM quotation doesn't seem
> particularly relevant.  With that fixed here and below patch is:
> 

The problem is that this quote is not present in IVB PRM, hence the
sentence: "Presumably the DevSNB behavior applies to IVB as well."

Looks like it is yet another omission in the PRMs. Adding Matt on Cc
just in case he wants to add something.

> Reviewed-by: Francisco Jerez <currojerez at riseup.net>
> 

Thanks!

Sam

>> +             *
>> +             * "For Align16 access mode, only encodings of 0000, 0010
>> +             *  and 0011 are allowed. Other codes are reserved."
>> +             *
>> +             * Presumably the DevSNB behavior applies to IVB as well.
>> +             */
>>              brw_inst_set_src0_vstride(devinfo, inst, BRW_VERTICAL_STRIDE_4);
>> -	 else
>> +         } else {
>>              brw_inst_set_src0_vstride(devinfo, inst, reg.vstride);
>> +         }
>>        }
>>     }
>>  }
>> @@ -594,13 +606,25 @@ brw_set_src1(struct brw_codegen *p, brw_inst *inst, struct brw_reg reg)
>>           brw_inst_set_src1_da16_swiz_w(devinfo, inst,
>>              BRW_GET_SWZ(reg.swizzle, BRW_CHANNEL_W));
>>  
>> -	 /* This is an oddity of the fact we're using the same
>> -	  * descriptions for registers in align_16 as align_1:
>> -	  */
>> -	 if (reg.vstride == BRW_VERTICAL_STRIDE_8)
>> +         if (reg.vstride == BRW_VERTICAL_STRIDE_8) {
>> +            /* This is an oddity of the fact we're using the same
>> +             * descriptions for registers in align_16 as align_1:
>> +             */
>> +            brw_inst_set_src1_vstride(devinfo, inst, BRW_VERTICAL_STRIDE_4);
>> +         } else if (devinfo->gen == 7 && !devinfo->is_haswell &&
>> +                    reg.type == BRW_REGISTER_TYPE_DF &&
>> +                    reg.vstride == BRW_VERTICAL_STRIDE_2) {
>> +            /* From HSW PRM:
>> +             *
>> +             * "For Align16 access mode, only encodings of 0000, 0010
>> +             *  and 0011 are allowed. Other codes are reserved."
>> +             *
>> +             * Presumably the DevSNB behavior applies to IVB as well.
>> +             */
>>              brw_inst_set_src1_vstride(devinfo, inst, BRW_VERTICAL_STRIDE_4);
>> -	 else
>> +         } else {
>>              brw_inst_set_src1_vstride(devinfo, inst, reg.vstride);
>> +         }
>>        }
>>     }
>>  }
>> -- 
>> 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: 862 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170307/afc2705c/attachment.sig>