On 07/03/17 22:39, Francisco Jerez wrote:
> Samuel Iglesias Gonsálvez <sigles...@igalia.com> writes:
>
>> On 04/03/17 01:44, Francisco Jerez wrote:
>>> Samuel Iglesias Gonsálvez <sigles...@igalia.com> writes:
>>>
>>>> From: Matt Turner <matts...@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 <sigles...@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."
>>
>
> In that case wouldn't it make sense to quote the SNB docs which are the
> ones you claim have any relevance for IVB?
>
Right. Fixed locally.
Sam
>> 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 <curroje...@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@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
signature.asc
Description: OpenPGP digital signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
