[Mesa-dev] [PATCH] i965/vec4: don't copy ATTR into 3src instructions with complex swizzles

[Matt Turner]

On Thu, Feb 4, 2016 at 10:55 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Thu, Feb 4, 2016 at 1:47 PM, Matt Turner <mattst88 at gmail.com> wrote:
>> The vec4 backend, at the end, does this:
>>
>>     if (inst->is_3src()) {
>>        for (int i = 0; i < 3; i++) {
>>           if (inst->src[i].vstride == BRW_VERTICAL_STRIDE_0)
>>              assert(brw_is_single_value_swizzle(inst->src[i].swizzle));
>>
>> So make sure that we use the same conditions when trying to
>> copy-propagate. UNIFORMs will be converted to vstride 0 in
>> convert_to_hw_regs, but so will ATTRs when interleaved (as will happen
>> in a GS with multiple attributes). Since the vstride is not set at
>> copy-prop time, infer it by inspecting dispatch_mode and reject ATTRs if
>> they have non-scalar swizzles and are interleaved.
>>
>> Fixes assertion errors in dolphin-generated geometry shaders (or
>> misrendering on opt builds) on Sandybridge or on IVB/HSW with
>> INTEL_DEBUG=nodualobj.
>
> I'm sure you realize this, but it also fixes unknown fails on IVB/HSW
> with instanced geometry shaders. Dunno if it's worth mentioning
> though.
>
>>
>> Co-authored-by: Ilia Mirkin <imirkin at alum.mit.edu>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93418
>> Cc: "11.0 11.1" <mesa-stable at lists.freedesktop.org>
>> ---
>> Ilia, I'm happy to change authorship back to you if you want.
>>
>> This version has no shader-db regressions. The initial version had these
>> results on HSW:
>>
>> total instructions in shared programs: 7129034 -> 7151511 (0.32%)
>> instructions in affected programs: 1092053 -> 1114530 (2.06%)
>> helped: 0
>> HURT: 11698
>
> OUCH! That's pretty hurt. I didn't anticipate this came up so often :(
>
> I don't really care about the authorship.

Thanks a bunch for figuring out the cause and sending the initial
patch. That made the rest of it super simple for me.

>>
>>
>>  src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
>> index c6f0b0d..6bd9928 100644
>> --- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
>> @@ -254,8 +254,8 @@ try_constant_propagate(const struct brw_device_info *devinfo,
>>
>>  static bool
>>  try_copy_propagate(const struct brw_device_info *devinfo,
>> -                   vec4_instruction *inst,
>> -                   int arg, struct copy_entry *entry)
>> +                   vec4_instruction *inst, int arg,
>> +                   struct copy_entry *entry, int attributes_per_reg)
>>  {
>>     /* Build up the value we are propagating as if it were the source of a
>>      * single MOV
>> @@ -320,7 +320,8 @@ try_copy_propagate(const struct brw_device_info *devinfo,
>>     unsigned composed_swizzle = brw_compose_swizzle(inst->src[arg].swizzle,
>>                                                     value.swizzle);
>>     if (inst->is_3src() &&
>> -       value.file == UNIFORM &&
>> +       (value.file == UNIFORM ||
>> +        (value.file == ATTR && attributes_per_reg != 1)) &&
>>         !brw_is_single_value_swizzle(composed_swizzle))
>>        return false;
>>
>> @@ -395,6 +396,11 @@ try_copy_propagate(const struct brw_device_info *devinfo,
>>  bool
>>  vec4_visitor::opt_copy_propagation(bool do_constant_prop)
>>  {
>> +   /* If we are in dual instanced or single mode, then attributes are going
>> +    * to be interleaved, so one register contains two attribute slots.
>> +    */
>> +   const int attributes_per_reg =
>> +      prog_data->dispatch_mode == DISPATCH_MODE_4X2_DUAL_OBJECT ? 1 : 2;
>
> I looked at this myself too, but just to confirm my reading of the
> code - you expect dispatch_mode to be set to 4X2_DUAL_OBJECT for
> regular vec4 vs on all chips? And will always be 4X1_SINGLE for gen6
> gs?

Exactly.

> Assuming my understanding above is correct, this is
>
> Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>
>
> Not sure if I get to do that when I'm in a Co-authored tag... but oh well.

I'll take it!