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

[Francisco Jerez]

Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:

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

In that case wouldn't it make sense to quote the SNB docs which are the
ones you claim have any relevance for IVB?

> 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: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170307/8a65ad25/attachment.sig>