Re: [PATCH] Enhance phiopt to handle BIT_AND_EXPR

[Jeff Law <law at redhat dot com>]

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.

Thanks!

jeff