Re: [PING][PATCH][REVISED] Fix PR middle-end/PR28690, modify swap_commutative_operands_p

["H.J. Lu" <hjl at lucon dot org>]

On Fri, Jul 13, 2007 at 04:03:33PM -0500, Peter Bergner wrote:
> 
> There are only two independent parts to the patch.  There is the
> swap_commutative_operands_p change that sorts via REGNO:
> 
> +  /* Group together equal REGs to do more simplification.  */
> +  if (REG_P (x) && REG_P (y))
> +    return REGNO (x) > REGNO (y);
> 
> This part of the patch isn't necessary to get the correct operand
> ordering for indexed load/store insns.  It was added at Paolo's
> suggestion because, in theory, it should help enable some extra
> canonicalizations.  It also brings it in line with the simplifications
> done by simplify_plus_minus_op_data_cmp.  I was actually going to post
> a follow on patch that replaces the simplify_plus_minus_op_data_cmp
> calls with calls to swap_commutative_operands_p, since they are now
> equivalent (with the patch).  As Jakub found on Red Hat's 4.1 branch,
> if they get out of sync and they start using different criteria for
> swapping operands, bad things can happen.  I think going to a single
> function is a good idea to ensure they never do get out of sync.

Why not break it into 2 parts with simplify_plus_minus_op_data_cmp
calling swap_commutative_operands_p?

> 
> The remainder of the patch isn't really a collection of independent
> parts.  If we remove one part, then we'll starting failing to correctly
> order the operands for indexed load/store operands.  As mentioned in

What do you mean by "failing"? What other changes does this

Index: tree-ssa-address.c
===================================================================
--- tree-ssa-address.c  (revision 125863)
+++ tree-ssa-address.c  (working copy)
@@ -125,7 +125,7 @@ gen_addr_rtx (rtx symbol, rtx base, rtx 
   if (base)
     {   
       if (*addr)
-       *addr = gen_rtx_PLUS (Pmode, *addr, base);
+       *addr = simplify_gen_binary (PLUS, Pmode, base, *addr);
       else
        *addr = base;
     }   

depend on?

> a previous email, for one particular SPEC benchmark (galgel), we get
> a nearly 500% performance degradation if we get this wrong, and as
> mainline stands now (without the patch) we are getting this wrong.
> 

I assume by "wrong", you meant very bad performance on Power 6, not
 nessarily codes which generate incorrect results.

Changes in commutative_operand_precedence may be partially responsible
for performance degradation on x86. How did you come up with those
numbers? What are the absolute minium changes for
commutative_operand_precedence? Your ChangeLog says

       * rtlanal.c (commutative_operand_precedence): Prefer both REG_POINTER
        and MEM_POINTER operands over REG and MEM operands.

It looks more than that.


H.J.