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

[Ilia Mirkin]

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.

>
>
>  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?

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.

  -ilia

>     bool progress = false;
>     struct copy_entry entries[alloc.total_size];
>
> @@ -465,7 +471,7 @@ vec4_visitor::opt_copy_propagation(bool do_constant_prop)
>           if (do_constant_prop && try_constant_propagate(devinfo, inst, i, &entry))
>              progress = true;
>
> -        if (try_copy_propagate(devinfo, inst, i, &entry))
> +         if (try_copy_propagate(devinfo, inst, i, &entry, attributes_per_reg))
>             progress = true;
>        }
>
> --
> 2.4.10
>