RE: [PATCH] Enhance phiopt to handle BIT_AND_EXPR

["Zhenqiang Chen" <zhenqiang dot chen at arm dot com>]

> -----Original Message-----
> From: Jeff Law [mailto:law@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.chen@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