> -----Original Message----- > From: Jeff Law [mailto:l...@redhat.com] > Sent: Wednesday, October 09, 2013 5:00 AM > To: Andrew Pinski; Zhenqiang Chen > Cc: GCC Patches > Subject: Re: [PATCH] Enhance phiopt to handle BIT_AND_EXPR > > 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.
Thank you! -Zhenqiang
