[Mesa-dev] [PATCH v3] i965/vec4: Change types as needed to propagate source modifiers using current instruction

[Jason Ekstrand]

On Sep 18, 2015 10:09 AM, "Alejandro Piñeiro" <apinheiro at igalia.com> wrote:
>
> SEL and MOV instructions, as long as they don't have source modifiers, are
> just copying bits around.  So those kind of instruction could be
propagated
> even if there are type mismatches. This is needed because NIR generates
> integer SEL and MOV instructions whenever it doesn't know what else to
> generate.
>
> This commit adds support for copy propagation using current instruction
> as reference.
>
> v2: include check for saturate, as Jason Ekstrand suggested
> v3: check that the dst.type and the src type are the same, in order to
>     solve (among others) the following deqp regression with v2:
>
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.lowp_uint_vertex

Reviewed-by: Jason Ekstrand <jason.ekstrand at intel.com>

The other will take more thinking.

> ---
>
> All the patches I sent to this series were tested against a full piglit
run.
> But while I was waiting for the review, I decided to run a full dEQP run,
and
> I found that v2 regressed on the following tests:
>   *
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.lowp_uint_vertex
>   *
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.lowp_uvec2_vertex
>   *
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.lowp_uvec3_vertex
>   *
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.lowp_uvec4_vertex
>   *
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.mediump_uint_vertex
>   *
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.mediump_uvec2_vertex
>   *
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.mediump_uvec3_vertex
>   *
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.mediump_uvec4_vertex
>   *
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uint_vertex
>   *
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uvec2_vertex
>   *
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uvec3_vertex
>   *
dEQP-GLES3.functional.shaders.operator.unary_operator.minus.highp_uvec4_vertex
>   * dEQP-GLES3.functional.shaders.matrix.mul.const.lowp_mat3x2_mat3_vertex
>
> On those tests we found situation like this:
>    7: mov vgrf6.0.x:D, -vgrf5.xxxx:D
>    8: mov vgrf0.0.x:F, vgrf6.xxxx:UD
>
> That with v2 was propagated as:
>    7: mov vgrf6.0.x:D, -vgrf5.xxxx:D
>    8: mov vgrf0.0.x:D, -vgrf5.xxxx:D
>
> Causing the regression. It was fixed by checking too that the type of the
> destination and the source are the same.
>
>  .../drivers/dri/i965/brw_vec4_copy_propagation.cpp | 30
++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 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 5a15eb8..1522eea 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_copy_propagation.cpp
> @@ -249,6 +249,18 @@ try_constant_propagate(const struct brw_device_info
*devinfo,
>  }
>
>  static bool
> +can_change_source_types(vec4_instruction *inst)
> +{
> +   return inst->dst.type == inst->src[0].type &&
> +      !inst->src[0].abs && !inst->src[0].negate && !inst->saturate &&
> +      (inst->opcode == BRW_OPCODE_MOV ||
> +       (inst->opcode == BRW_OPCODE_SEL &&
> +        inst->dst.type == inst->src[1].type &&
> +        inst->predicate != BRW_PREDICATE_NONE &&
> +        !inst->src[1].abs && !inst->src[1].negate));
> +}
> +
> +static bool
>  try_copy_propagate(const struct brw_device_info *devinfo,
>                     vec4_instruction *inst,
>                     int arg, struct copy_entry *entry)
> @@ -308,7 +320,9 @@ try_copy_propagate(const struct brw_device_info
*devinfo,
>          value.swizzle != BRW_SWIZZLE_XYZW) &&
!inst->can_do_source_mods(devinfo))
>        return false;
>
> -   if (has_source_modifiers && value.type != inst->src[arg].type)
> +   if (has_source_modifiers &&
> +       value.type != inst->src[arg].type &&
> +       !can_change_source_types(inst))
>        return false;
>
>     if (has_source_modifiers &&
> @@ -362,7 +376,19 @@ try_copy_propagate(const struct brw_device_info
*devinfo,
>        }
>     }
>
> -   value.type = inst->src[arg].type;
> +   if (has_source_modifiers &&
> +       value.type != inst->src[arg].type) {
> +      /* We are propagating source modifiers from a MOV with a different
> +       * type.  If we got here, then we can just change the source and
> +       * destination types of the instruction and keep going.
> +       */
> +      assert(can_change_source_types(inst));
> +      for (int i = 0; i < 3; i++) {
> +         inst->src[i].type = value.type;
> +      }
> +      inst->dst.type = value.type;
> +   } else
> +      value.type = inst->src[arg].type;
>     inst->src[arg] = value;
>     return true;
>  }
> --
> 2.1.4
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150918/489910a2/attachment.html>