This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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.

Thanks!

jeff



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]