On Sep 18, 2015 10:09 AM, "Alejandro Piñeiro" <apinhe...@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.ekstr...@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
