On 09/30/13 09:57, Andrew Pinski wrote:
On Mon, Sep 30, 2013 at 2:29 AM, Zhenqiang Chen <zhenqiang.c...@arm.com> wrote:
Hi,
The patch enhances phiopt to handle cases like:
if (a == 0 && (...))
return 0;
return a;
Boot strap and no make check regression on X86-64 and ARM.
Is it OK for trunk?
From someone who wrote lot of this code (value_replacement in fact),
this looks good, though I would pull:
+ if (TREE_CODE (gimple_assign_rhs1 (def)) == SSA_NAME)
+ {
+ gimple def1 = SSA_NAME_DEF_STMT (gimple_assign_rhs1 (def));
+ if (is_gimple_assign (def1) && gimple_assign_rhs_code (def1) == EQ_EXPR)
+ {
+ tree op0 = gimple_assign_rhs1 (def1);
+ tree op1 = gimple_assign_rhs2 (def1);
+ if ((operand_equal_for_phi_arg_p (arg0, op0)
+ && operand_equal_for_phi_arg_p (arg1, op1))
+ || (operand_equal_for_phi_arg_p (arg0, op1)
+ && operand_equal_for_phi_arg_p (arg1, op0)))
+ {
+ *code = gimple_assign_rhs_code (def1);
+ return 1;
+ }
+ }
+ }
Out into its own function since it is repeated again for
gimple_assign_rhs2 (def).
Agreed. Repeating blobs of code like that should be pulled out into its
own subroutine.
It's been 10 years since we wrote that code Andrew and in looking at it
again, I wonder if/why DOM doesn't handle the stuff in
value_replacement. It really just looks like it's propagation of an
edge equivalence. do you recall the motivation behind value_replacement
and why we didn't just let DOM handle it?
Also what about cascading BIT_AND_EXPR
like:
if((a == 0) & (...) & (...))
I notice you don't handle that either.
Well, given the structure of how that code is going to look, it'd just
be repeated walking through the input chains. Do-able, yes. But I
don't think handling that should be a requirement for integration.
I'll go ahead and pull the common bits into a single function and commit
on Zhenqiang's behalf.
Thanks!
jeff
