On 11/11/13 10:50, Richard Biener wrote:
> On Sat, 9 Nov 2013, Tom de Vries wrote:
>> Bootstrapped and regtested on x86_64.
>>
>> OK for trunk?
>
> Comments inline
>
>
> diff --git a/gcc/tree-ssa-tail-merge.c b/gcc/tree-ssa-tail-merge.c
> index 98b5882..43516a7 100644
> --- a/gcc/tree-ssa-tail-merge.c
> +++ b/gcc/tree-ssa-tail-merge.c
> @@ -1173,8 +1173,47 @@ gimple_equal_p (same_succ same_succ, gimple s1, gimple
> s2)
> lhs2 = gimple_get_lhs (s2);
> if (TREE_CODE (lhs1) != SSA_NAME
> && TREE_CODE (lhs2) != SSA_NAME)
> - return (vn_valueize (gimple_vdef (s1))
> - == vn_valueize (gimple_vdef (s2)));
> + {
> + /* If the vdef is the same, it's the same statement. */
> + if (vn_valueize (gimple_vdef (s1))
> + == vn_valueize (gimple_vdef (s2)))
> + return true;
> +
> + /* If the vdef is not the same but the vuse is the same, it's not the
> + same stmt. */
> + if (vn_valueize (gimple_vuse (s1))
> + == vn_valueize (gimple_vuse (s2)))
> + return false;
>
Richard,
> What's the logic behind this?
> We want to use VN to get more "positive"
> results
We can use it to get both positive and negative results faster.
> - doing a negative early out looks suspicious to me ...
>
If the vdefs are the same, the VN has concluded that the statements are the
same. We have a positive early out.
If the vdefs are not the same, the VN has concluded that the statements are
different. But if the vuses are the same, the VN has concluded that the
statements are different because of structural difference. Which means there's
no sense in us repeating the structural comparison. We have a negative early
out.
If the vdefs are not the same, the VN has concluded that the statements are
different. But if the vuses are different, the VN may have concluded that the
statements are different because of the different vuses. So it still makes sense
to try structural comparison.
> + /* If the vdef is not the same and the vuse is not the same, it might
> be
> + same stmt. */
> +
> + /* Test for structural equality. */
> + if (gimple_assign_rhs_code (s1) != gimple_assign_rhs_code (s1)
>
> s2
>
> + || (gimple_assign_nontemporal_move_p (s1)
> + != gimple_assign_nontemporal_move_p (s2)))
>
> I don't think we should care (it'll be false - a later pass sets it,
> it's an optimization hint, not a correctness issue). More interesting
> would be to assert that has_volatile_ops is the same if the operands
> turned out to be the same.
>
OK.
> + return false;
> +
> + if (!operand_equal_p (lhs1, lhs2, 0))
> + return false;
> +
> + t1 = gimple_assign_rhs1 (s1);
> + t2 = gimple_assign_rhs1 (s2);
> + if (!gimple_operand_equal_value_p (t1, t2))
> + return false;
> +
> + t1 = gimple_assign_rhs2 (s1);
> + t2 = gimple_assign_rhs2 (s2);
> + if (!gimple_operand_equal_value_p (t1, t2))
> + return false;
> +
> + t1 = gimple_assign_rhs3 (s1);
> + t2 = gimple_assign_rhs3 (s2);
> + if (!gimple_operand_equal_value_p (t1, t2))
> + return false;
>
> for (i = 1; i < gimple_num_ops (s1); ++i)
> t1 = gimple_op (s1, i);
> ...
>
> but I think you should only compare rhs1 and thus only handle
> GIMPLE_ASSIGN_SINGLEs this way - the others have a SSA name
> lhs.
>
I see.
> That makes the whole thing just
>
> if (TREE_CODE (lhs1) != SSA_NAME
> && TREE_CODE (lhs2) != SSA_NAME)
> {
> if (vn_valueize (gimple_vdef (s1))
> == vn_valueize (gimple_vdef (s2)))
> return true;
> return operand_equal_p (lhs1, lhs2, 0)
> && gimple_operand_equal_value_p (gimple_assign_rhs1 (s1),
> gimple_assign_rhs2 (s2));
> }
>
> Ok with doing it this way.
I'll retest and checkin like this, unless you agree about the early out, which I
still think is a good idea, although the structural test is now much smaller.
Thanks,
- Tom
>
> Thanks,
> Richard.
>
> + /* Same structure. */
> + return true;
> + }
> else if (TREE_CODE (lhs1) == SSA_NAME
> && TREE_CODE (lhs2) == SSA_NAME)
> return vn_valueize (lhs1) == vn_valueize (lhs2);
>
