"duanbo (C)" <duan...@huawei.com> writes:
> @@ -4395,6 +4395,40 @@ vect_recog_mask_conversion_pattern (vec_info *vinfo,
> {
> tmp = vect_recog_temp_ssa_var (TREE_TYPE (rhs1), NULL);
> pattern_stmt = gimple_build_assign (tmp, rhs1);
> + tree rhs1_op0 = TREE_OPERAND (rhs1, 0);
> + tree rhs1_op1 = TREE_OPERAND (rhs1, 1);
I think we also need to handle this case when picking the vector
types and deciding whether a pattern is needed. Otherwise it would
be possible for rhs1_op1 to be the only input that requires a different
mask, and we'd wrongly avoid creating a conversion for it.
> + if (rhs1_op0 && rhs1_op1
> + && (TREE_CODE (TREE_TYPE (rhs1_op0)) == BOOLEAN_TYPE)
> + && (TREE_CODE (TREE_TYPE (rhs1_op1)) == BOOLEAN_TYPE))
Minor formatting nit, but the brackets around the == are redundant
(but are still kept for multiline comparisons like the == below).
> + {
> + tree rhs1_op0_type = integer_type_for_mask (rhs1_op0, vinfo);
> + tree rhs1_op1_type = integer_type_for_mask (rhs1_op1, vinfo);
> + enum tree_code rhs1_code = gimple_assign_rhs_code (pattern_stmt);
> + if (rhs1_op0_type && rhs1_op1_type
> + && (!(TYPE_PRECISION (rhs1_op0_type)
> + == TYPE_PRECISION (rhs1_op1_type))))
More obvious as != rather than !(… == …).
> + {
> + if (TYPE_PRECISION (rhs1_op0_type)
> + < TYPE_PRECISION (rhs1_op1_type))
> + {
> + vectype2
> + = get_mask_type_for_scalar_type (vinfo, rhs1_op0_type);
> + if (vectype2)
> + rhs1_op1 = build_mask_conversion (vinfo, rhs1_op1,
> + vectype2, stmt_vinfo);
It isn't safe to ignore null returns like this. We should bail out
instead.
> + }
> + else
> + {
> + vectype2
> + = get_mask_type_for_scalar_type (vinfo, rhs1_op1_type);
> + if (vectype2)
> + rhs1_op0 = build_mask_conversion (vinfo, rhs1_op0,
> + vectype2, stmt_vinfo);
> + }
This seems to go for the narrower of the two precisions. Are you sure
that's always the optimal choice? Gut instinct says that we should
move in the direction of vectype1, since that's what the result will
be converted to. E.g. maybe:
- if both precisions are less than vectype1's precision or both are
more than vectype1's precision, pick the closest to vectype1.
- otherwise, convert both inputs to vectype1 individually, rather than
converting the result to vectype1.
I admit I haven't thought about it much though.
> + pattern_stmt = gimple_build_assign (tmp, rhs1_code,
> + rhs1_op0, rhs1_op1);
> + }
This statement is redundant with the one created above. I think instead
we should create the statement this way in all cases, even if rhs1_op0
and rhs1_op1 don't change. That's effectively what the:
gimple_build_assign (tmp, rhs1)
does anyway.
I'm going to be away next week so my next reply might be even slower
than usual, sorry. Would be happy for someone else to review in the
meantime though. :-)
Thanks,
Richard
