On Sep 18, 2015 10:09 AM, "Alejandro Piñeiro" <<a href="mailto:apinheiro@igalia.com">apinheiro@igalia.com</a>> 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 <<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a>> 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 > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a>