GCC Bugzilla Bug 28690

On some powerpc processors, it is very desirable for performance reasons, to
have the base pointer for an indexed load/store insn to be in the rA position
rather than the rB position (example insn shown below).

    lwzx rD,rA,rB

For some test cases, we get this right, but for the following test case, we get
it wrong (regardless of -m32 or -m64):

int indexedload (int *x, int i)
{
  return x[i];
}

Results in the following powerpc asm:

indexedload:
        slwi r4,r4,2
        lwzx r3,r4,r3   # We want r3,r3,r4
        blr

Dan Berlin tracked one problem down to the reassociation pass ignoring non
integral types like pointers.  His patch (I'll let Dan attach it here) fixed
the ordering of the address calc at the end of tree-ssa, but the rtl expanders
seem to be undoing this change so we still end up with the wrong ordering on
the lwzx insn.

Confirmed

Subject: Re:  [4.2 Regression] Performace problem with
 indexed load/stores on powerpc


Here is the reassoc patch that puts them in the right order at the tree
level.

Index: tree-ssa-reassoc.c
===================================================================
--- tree-ssa-reassoc.c  (revision 115962)
+++ tree-ssa-reassoc.c  (working copy)
@@ -356,6 +356,13 @@ sort_by_operand_rank (const void *pa, co
       && TREE_CODE (oeb->op) == SSA_NAME)
     return SSA_NAME_VERSION (oeb->op) - SSA_NAME_VERSION (oea->op);

+  /* For pointers, most things want the *base* pointer to go first to
+     try indexed loads. The base pointer is the one with the *lesser*
+     rank.  For everything else, put them in order from greatest rank
+     to least.  */
+  if (POINTER_TYPE_P (TREE_TYPE (oea->op)))
+    return oea->rank - oeb->rank;
+
   return oeb->rank - oea->rank;
 }

@@ -1309,7 +1316,9 @@ reassociate_bb (basic_block bb)
       if (TREE_CODE (stmt) == MODIFY_EXPR)
        {
          tree lhs = TREE_OPERAND (stmt, 0);
+         tree lhst = TREE_TYPE (lhs);
          tree rhs = TREE_OPERAND (stmt, 1);
+         tree rhst = TREE_TYPE (rhs);

          /* If this was part of an already processed tree, we don't
             need to touch it again. */
@@ -1318,10 +1327,10 @@ reassociate_bb (basic_block bb)

          /* If unsafe math optimizations we can do reassociation for
             non-integral types.  */
-         if ((!INTEGRAL_TYPE_P (TREE_TYPE (lhs))
-              || !INTEGRAL_TYPE_P (TREE_TYPE (rhs)))
-             && (!SCALAR_FLOAT_TYPE_P (TREE_TYPE (rhs))
-                 || !SCALAR_FLOAT_TYPE_P (TREE_TYPE(lhs))
+         if (((!INTEGRAL_TYPE_P (lhst) & !POINTER_TYPE_P (lhst))
+              || (!INTEGRAL_TYPE_P (rhst) && !POINTER_TYPE_P (rhst)))
+             && (!SCALAR_FLOAT_TYPE_P (rhst)
+                 || !SCALAR_FLOAT_TYPE_P (lhst)
                  || !flag_unsafe_math_optimizations))
            continue;

Ok, I tracked down where the expander is swapping the operands. It's occuring
at simplify-rtx.c:simplify_binary_operation() at line 1459:

  /* Make sure the constant is second.  */
  if (GET_RTX_CLASS (code) == RTX_COMM_ARITH
      && swap_commutative_operands_p (op0, op1))
    {
      tem = op0, op0 = op1, op1 = tem;
    }

In this particular case, op0 = (reg/v/f:SI 120 [ base ]) and
op1 = (mult:SI (reg/v:SI 121 [ offset ])
    (const_int 4 [0x4]))

[src being: int indexedload (int *base, int offset) { return base[offset]; }]

swap_commutative_operands_p(op0,op1) simply returns:
commutative_operand_precedence (op0) < commutative_operand_precedence (op1),
which ends up being "-1 < 4", so we swap the operands.  For powerpc, we'd
prefer the base pointer remain the first operand for performance reasons. I'd
like other people familar with this code to comment on how we can fix this. 
One could simply bump up the priority of base pointers (ie, "case RTX_OBJ:"),
but I personally don't know how that would affect other platforms.

> which ends up being "-1 < 4", so we swap the operands.  For powerpc, we'd
> prefer the base pointer remain the first operand for performance reasons. I'd
> like other people familar with this code to comment on how we can fix this. 
> One could simply bump up the priority of base pointers (ie, "case RTX_OBJ:"),
> but I personally don't know how that would affect other platforms.

Very much.  The canonical form enforced by swap_commutative_operands_p is
relied upon by all the code for simplifications, that expects for example a
(plus (mult A B) C) and not a (plus C (mult A B)).

If one took care to fix all of them, it could work, but it's no easy feat. :-(

I think the best solution (if it works) is to put this transformation in
rs6000's legitimize_address. Given a (plus (mult A B) C), force the mult into a
pseudo (let's call it D) and then return (plus C D) with the operands swapped.

Created an attachment (id=12190) [edit]
Patch to rs6000_legitimize_address to force base pointers into rA position of
indexed load/store instructions.

Ok, taking Paolo's suggestion of moving the change into
rs6000_legititmize_address, I'm trying the attached patch which bootstraps fine
and fixes the base pointer order for us.  I'm running the testsuite now and
will report back when it's done.

        * config/rs6000/rs6000.c (rs6000_legitimize_address): For performance
        reasons, force PLUS register operands that are base pointers to be the
        first operand.

To clarify, I make this suggestion because I think that we were getting it
right pre-4.2 just out of luck.

I also thought about having a lower commutative_operand_precedence for
REG_POINTER regs than normal regs, but in fact regs need to have the same
precedence as other rtx's of class RTX_OBJ, or you get pessimization on x86
(e.g. on crafty).

Well, to get REG_POINTER regs to be the first operand, we'd need to increase
their commutative_operand_precedence.  I tried that change already, but that
led to an infinite recursion loop while attempting to simplify the rtl. As you
said, the current ordering seems to be relied upon by the code for
simplifications.

My rs6000_legitimize_address change is still running through the testsuite.

Ok, this also passed regression tests on powerpc64-linux (32-bit and 64-bit
testsuite runs) for c, c++, fortran, objc, obj-c++ and java.

Does the attached patch look reasonable to everyone?

Created an attachment (id=12305) [edit]
Patch to rs6000_legitimize_address to force base pointers into rA position of
indexed load/store instructions.

It seems -msoft-float doesn't like the operand swapping, so this patch disables
it when we specify -msoft-float on the compile command.

(From update of attachment 12190 [edit])
Forgot to obsolete this patch by the updated patch.

Created an attachment (id=12306) [edit]
Alternate patch to rs6000_legitimize_address to force base pointers into rA
position of indexed load/store instructions.

Same as the other patch, except we don't call force_reg() on constants.

Anton dicovered that we don't get multiple dimensioned arrays like the
following test case:

int indexedload(int ***base, int idx0, int idx1, int idx2)
{
  return base[idx0][idx1][idx2];
}

This one leads to 3 indexed loads.  We transform the first indexed load ok, but
the other two we don't.  I tracked that down to force_reg (called from
break_out_memory_refs) doesn't propagate the MEM_POINTER flag to a REG_POINTER
flag on the reg it creates.  I posted/commited a fix which was approved:

    http://gcc.gnu.org/ml/gcc-patches/2006-09/msg00941.html

We now successfully transform all of the indexed loads in this test case now.

Yet another test case from Anton we don't catch.  Will they never end?!?! ;)

int indexedload(int *base, int len)
{
  int i, sum = 0;
  for (i=0; i < len; i++)
    sum += base[i];
  return sum;
}

In this case, LEGITIMIZE_ADDRESS cannot help, because it is never passed an
operand that includes the base pointer.  Instead, we're passed a pseudo
register that was set previously to calculation using the base pointer, so in
this case, we can't propagate the REG_POINTER flag.

(In reply to comment #13)
> Yet another test case from Anton we don't catch.  Will they never end?!?! ;)
I bet a beer or a shot of vodka, that this is caused by MEM_REF not expanding
with a REG_POINTER.

Subject: Re:  [4.2 Regression] Performace problem
        with indexed load/stores on powerpc

On Fri, 2006-09-22 at 17:05 +0000, pinskia at gcc dot gnu dot org wrote:
> 
> ------- Comment #14 from pinskia at gcc dot gnu dot org  2006-09-22 17:05 -------
> (In reply to comment #13)
> > Yet another test case from Anton we don't catch.  Will they never end?!?! ;)
> I bet a beer or a shot of vodka, that this is caused by MEM_REF not expanding
> with a REG_POINTER.

And I lost because we have:
;; sum = sum + MEM[base: base, index: (int *) i * 4B]
(insn 29 27 30 (set (reg:SI 128)
        (ashift:SI (reg/v:SI 123 [ i ])
            (const_int 2 [0x2]))) -1 (nil)
    (nil))

(insn 30 29 31 (set (reg:SI 129)
        (mem:SI (plus:SI (reg:SI 128)
                (reg/v/f:SI 125 [ base ])) [0 S4 A32])) -1 (nil)
    (nil))

(insn 31 30 0 (set (reg/v:SI 122 [ sum ])
        (plus:SI (reg/v:SI 122 [ sum ])
            (reg:SI 129))) -1 (nil)
    (nil))



-- Pinski

Out of curiosity, which powerpc processors are affected by this?

-Chris

Created an attachment (id=12375) [edit]
Patch to swap_commutative_operands_p and gen_addr_rtx to force base pointers
into rA position of indexed load/store instructions.

We propagated the MEM_POINTER/REG_POINTER flags fine.  The problem is that the
memory reference we're handed is a REG + REG which looks legitimate to us, so
we never call LEGITIMIZE_ADDRESS, so we never have a chance to swap the
operands.

Since we cannot fixup the latest test case in LEGITIMIZE_ADDRESS, I've decided
to attempt another swap_commutative_operands_p() /
commutative_operand_precedence() change.  However, I'm a little more selective
on when we change swap_commutative_operands_p()'s return value.  With this
patch, I'm able to transform each of the test cases so that the base address if
the first operand of the indexed load.

        * rtlanal.c (swap_commutative_operands_p): Preference a REG_POINTER
        over a non REG_POINTER.
        * tree-ssa-address.c (gen_addr_rtx): Force a REG_POINTER to be
        the first operand of a PLUS.

Subject: Re:  [4.2 Regression] Performace problem with
 indexed load/stores on powerpc


>         * rtlanal.c (swap_commutative_operands_p): Preference a REG_POINTER
>         over a non REG_POINTER.
>         * tree-ssa-address.c (gen_addr_rtx): Force a REG_POINTER to be
>         the first operand of a PLUS.
This is more gentle indeed.  Be careful however as functions calling 
commutative_operand_precedence directly may have a problem with that.  
Can you try making an address illegitimate if it is non-REG_POINTER + 
REG_POINTER?  Or set up splitters to do the transformation just before 
reload?

Paolo

David has already said offline that he would reject any patch that would cause
us to view a non-REG_POINTER + REG_POINTER expression an not legitimate.  I
agree with that.

Sorry, but I'm slowly learning the machine description format.  How exactly
would adding a splitter help us?

Paolo, forcing all addresses through legitimize_address should not be the goal.
 The wrong ordering has performance effects, but is not an invalid address. 
While the performance effects on POWER-specific, canonicalizing addresses is a
general GCC issue.  GCC appears to want REG_POINTER first, but does not enforce
it.

I am willing to consider target-specific fixes as a last resort, but I do not
see any reason that GCC should not create and maintain a canonical address
ordering of REG_POINTER first.  Trying to correct this in the rs6000 backend is
a kludge.

Note that I don't oppose at all fixing the problem in
swap_commutative_operands_p.  At the very least, you have to change at the very
least simplify-rtx.c's uses of commutative_operand_precedence to use s_c_o_p
instead, but that's a minor problem.

I'm also worried of the interaction between this change to
swap_commutative_operands_p and swap_commutative_operands_with_target which
(even though I refactored it quite recently to happen very early, in expand) is
an optimization that CSE has performed for years and has big impact on x86, for
example on crafty.

Subject: Re:  [4.2 Regression] Performace problem with indexed load/stores on
powerpc 

        I am not suggesting that the problem has to be solved in
swap_commutative_operands, etc.  I would think that GCC should be able to
create commutative addresses where they are formed.

Subject: Bug 28690

Author: janis
Date: Thu Oct 12 17:23:10 2006
New Revision: 117668

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=117668
Log:
        PR middle-end/28690
        * explow.c (force_reg): Set REG_POINTER flag according to
        MEM_POINTER flag.

Modified:
    branches/ibm/gcc-4_1-branch/gcc/ChangeLog
    branches/ibm/gcc-4_1-branch/gcc/explow.c

Ok, Anton hit another test case the last patch doesn't transform.  It's
actually the same test case as in comment #13, except, "int *base" is now a
global variable rather than a function parameter.

int *base;

int indexedload(int len)
{
  int i, sum = 0;
  for (i=0; i < len; i++)
    sum += base[i];
  return sum;
}

With this case, we get the following RTL generated for the load of *base into a
register:

;; base.1 = base
(insn 24 22 25 (set (reg:SI 129)
        (high:SI (symbol_ref:SI ("base") [flags 0x84] <var_decl 0x400a6930
base>))) -1 (nil)
    (nil))

(insn 25 24 26 (set (reg/f:SI 128)
        (lo_sum:SI (reg:SI 129)
            (symbol_ref:SI ("base") [flags 0x84] <var_decl 0x400a6930 base>)))
-1 (nil)
    (expr_list:REG_EQUAL (symbol_ref:SI ("base") [flags 0x84] <var_decl
0x400a6930 base>)
        (nil)))

(insn 26 25 0 (set (reg:SI 124 [ base.1 ])
        (mem/c/i:SI (reg/f:SI 128) [3 base+0 S4 A32])) -1 (nil)
    (nil))

There seem to be two problems here.  First, the mem/c/i:SI (reg/f:SI 128) seems
to be missing a MEM_POINTER attribute.  I tracked that down to
rtl.h:MEM_COPY_ATTRIBUTES(LHS,RHS) not copying the MEM_POINTER attribute from
RHS to LHS.  I added the code to do that, so the mem above does get the
MEM_POINTER flag set. However, the reg:SI 124 [base.1] is still missing the
REG_POINTER flag.

This second problem seems to be a problem in expand_one_register_var(tree var).
 In this specific case, var seems to be a pointer type, but looks to be
"artifical", so we skip the code that would have marked the reg rtx with the
REG_POINTER:

Breakpoint 4, expand_one_register_var (var=0x400a6d90) at
/home/bergner/gcc/gcc-mainline-rtlanal/gcc/cfgexpand.c:643
643       tree type = TREE_TYPE (var);
(gdb) call debug_tree (var)
 <var_decl 0x400a6d90 base.1
    type <pointer_type 0x4009ae38
        type <integer_type 0x4009a340 int sizes-gimplified public SI
            size <integer_cst 0x4008e5e0 constant invariant 32>
            unit size <integer_cst 0x4008e2a0 constant invariant 4>
            align 32 symtab 0 alias set 2 precision 32 min <integer_cst
0x4008e580 -2147483648> max <integer_cst 0x4008e5a0 2147483647>
            pointer_to_this <pointer_type 0x4009ae38>>
        public unsigned SI size <integer_cst 0x4008e5e0 32> unit size
<integer_cst 0x4008e2a0 4>
        align 32 symtab 0 alias set 3>
    used unsigned ignored SI file indexedload5.c line 7 size <integer_cst
0x4008e5e0 32> unit size <integer_cst 0x4008e2a0 4>
    align 32 context <function_decl 0x40151f00 indexedload> chain <var_decl
0x400a6e00 D.1531>>
(gdb) p var->decl_common.artificial_flag
$8 = 1

If I clear var->decl_common.artificial_flag with the debugger and continue,
then we get the REG_POINTER flag set and the indexed load is generated with the
base pointer first like we want.  Does anyone know why var above was marked as
artifical?

(In reply to comment #24)
> Does anyone know why var above was marked as
> artifical?
Yes because it is an compiler generated decl for the load of the global.  The
main reason why we don't mark the RTL for artifical decls is because we get
still like:
(int*)int_var
Which causes problem.  This is what PTR_PLUS_EXPR which I am creating helps
solves.

The following patch was checked into mainline to fix the "first" problem
described in comment #24.

Author: bergner
Date: Sat Nov 11 04:20:37 2006
New Revision: 118684

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=118684
Log:
        * rtl.h (MEM_COPY_ATTRIBUTES): Copy MEM_POINTER.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/rtl.h

This case is still not fixed:


struct s {
  int size;
  float *data;
};

void f(struct s *d, struct s *s)
{
  int i;
  for (i = 0; i < s->size; i++)
    d->data[i] += s->data[i];
}

The body of the loop is compiled to:

L4:
        slwi r2,r9,2
        addi r9,r9,1
        lfsx f0,r2,r3
        lfsx f13,r4,r2
        fadds f0,f0,f13
        stfsx f0,r2,r3
        bdnz L4

Note how r2 is twice in the first position, and once in the second.

Another problem with the current patch, is we get one testsuite regression
(gfortran.fortran-torture/compile/defined_type_2.f90 at -O1).  For this simple
testcase, we end up generating bad assembler:

    mr 9,sfp

instead of:

    mr 9,1

For some reason, the stack frame pseudo isn't reloaded correctly and we spit
out the "sfp" text instead of the correct register number.

Talking with Andrew on IRC, he said the test case in comment #27 fails for the
same reason as the test case in comment #24 (ie, it looks like an artificial
decl) and should be fixed with his PTR_PLUS_EXPR work.

Ok, the problem from comment #28 was due to a latent bug in
reload1.c:eliminate_regs_in_insn(). The bug is that eliminate_regs_in_insn()
calls single_set() on the passed in insn.  This has been fine before, but now
with the patch, we end up passing in a parallel insn for a load with update and
the load portion of the parallel has the REG_UNUSED flag set.  This causes
single_set() to return the "update" portion of the parallel instead of
returning NULL as it would do normally with parallels.  This causes us to only
eliminate the update portion of the parallel and we skip eliminating the load
portion.  The problem insn belfore eliminate_regs_in_insn() looks like:

(insn 12 62 13 2 (parallel [
            (set (reg:SI 0 0 [125])
                (mem/s/j:SI (plus:SI (reg/f:SI 113 sfp)
                        (const_int 8 [0x8])) [0 S4 A32]))
            (set (reg/f:SI 9 9 [orig:124 D.965 ] [124])
                (plus:SI (reg/f:SI 113 sfp)
                    (const_int 8 [0x8])))
        ]) 373 {*movsi_update1} (nil)
    (expr_list:REG_UNUSED (reg:SI 0 0 [125])
        (nil)))

After eliminate_regs_in_insn(), we have:

(insn 12 62 13 2 (parallel [
            (set (reg:SI 0 0 [125])
                (mem/s/j:SI (plus:SI (reg/f:SI 113 sfp)
                        (const_int 8 [0x8])) [0 S4 A32]))
            (set (reg/f:SI 9 9 [orig:124 D.965 ] [124])
                (plus:SI (reg/f:SI 1 1)
                    (const_int 8 [0x8])))
        ]) 373 {*movsi_update1} (nil)
    (expr_list:REG_UNUSED (reg:SI 0 0 [125])
        (nil)))

However, calculate_needs_all_insns() ends up backing out the eliminated
(reg/f:SI 1 1) with the non eliminated (reg/f:SI 113 sfp) and  (reg/f:SI 113
sfp) never gets eliminated after that and we generate bogus assembler.

In addition to the latest patch attached here, I added the following patch to
stop eliminate-regs_in_insn from calling single_set for parallel insns.  It
fixed the bug here and bootstrapped and regtested with no errors.  I'll post
the  combined patch to gcc-patches.

...and here's the patch I mentioned in the previous comment:

Index: reload1.c
===================================================================
--- reload1.c   (revision 119497)
+++ reload1.c   (working copy)
@@ -2930,7 +2930,7 @@ eliminate_regs_in_insn (rtx insn, int re
   int icode = recog_memoized (insn);
   rtx old_body = PATTERN (insn);
   int insn_is_asm = asm_noperands (old_body) >= 0;
-  rtx old_set = single_set (insn);
+  rtx old_set;
   rtx new_body;
   int val = 0;
   int i;
@@ -2949,6 +2949,12 @@ eliminate_regs_in_insn (rtx insn, int re
       return 0;
     }

+  /* Guard against a PARALLEL with a REG_UNUSED note.  */
+  if (GET_CODE (PATTERN (insn)) != PARALLEL)
+    old_set = single_set (insn);
+  else
+    old_set = 0;
+
   if (old_set != 0 && REG_P (SET_DEST (old_set))
       && REGNO (SET_DEST (old_set)) < FIRST_PSEUDO_REGISTER)
     {

Another example, pared down from ammp benchmark in cpu2000. 

void f2(int *, int *);
void mm_fv_update_nonbon(void)
{
 int j, nx;
 int naybor[27];

 f2(naybor, &nx);

 for(j=0; j< 27; j++)
     if( naybor[j]) break; /* Indexed load problem */

My prior comment is missing the closing bracket for the procedure, but example
is otherwise complete.

Created an attachment (id=12915) [edit]
Patch to commutative_operand_precedence to increase the precedence of
REG_POINTER and MEM_POINTER objects.

This patch modifies commutative_operand_precedence to increase the precedence
of REG_POINTER and MEM_POINTER objects. This obviates the need for
swap_commutative_operands_with_target which has been replaced by a call to
swap_commutative_operands_p.

This patch improves performance on POWER6 (using -mcpu=power6) by 30% across
both specint and specfp, with a 498% improvement on galgel. On POWER5 (using
-mcpu=power5), there was only negligible differences between the base and
patched compilers. It also correctly transforms all of the above test cases
except those with "artificial" pointers which will have to wait until Andrew's
PTR_PLUS_EXPR work is complete.

Paolo tested this patch on x86 and saw 4% degradation on galgel which he said
he'd look into. However, overall across both specint and specfp, the
performance change didn't seem that bad.

Created an attachment (id=13042) [edit]
Alternate patch to commutative_operand_precedence to increase the precedence of
REG_POINTER and MEM_POINTER objects.

Ok, now that the libjava multilib problems have been fixed, I've been
able to attempt to bootstrap the patch in Comment #34 with java enabled.
In doing so, I'm now hitting an ICE while building the 64-bit libgcj.
The ICE is occurring in the same location and for the same reason as
the ICE (optabs.c:emit_cmp_and_jump_insns()) I hit when I attempted
to change swap_commutative_operands_p() (as in this alternate
patch) so that it sorted REG's by register numbers similar to how
simplify_plus_minus_op_data_cmp() sorts them.

With this attached alternate patch, we have a simple testcase that 
exposes the problem:

  void
  gomp_sem_wait_slow (int *sem, int a, int b)
  {
    __sync_bool_compare_and_swap (sem, a, b);
  }

For this testcase, the swap_commutative_operands_p (x, y) call that guards
the gcc_assert (label) we're failing in, "x" and "y" are simple REGs that
are swapped due to REGNO(y) is smaller then RGENO(x).  I don't understand
why the gcc_assert(label) is needed, but I'll try and track that down.

Created an attachment (id=13101) [edit]
Alternate patch to commutative_operand_precedence to increase the precedence of
REG_POINTER and MEM_POINTER objects.

With help from Honza, this updated patch eliminates the ICE the previous patch
hit.

Will not be fixed in 4.2.0; retargeting at 4.2.1.

Created an attachment (id=13671) [edit]
Updated patch to address x86_64 performance issues.

This updated patch reverts the swap_commutative_operands_with_target removal
from the previous patch, since it seems x86_64 still relies on this for some
benchmarks. The change doesn't seem to affect the POWER6 performance benefits
we were seeing with the older patch.

committed??

(In reply to comment #39)
> committed??

This is now more like a meta-bug, see the other two bugs which are opened for
the current issues (yes both are assigned to me and both are actively being
worked on, well one is depend on the other but still being worked on).

Subject: Re:  [4.2/4.3 Regression] Performace problem
 with indexed load/stores on powerpc


> This is now more like a meta-bug, see the other two bugs which are opened for
> the current issues (yes both are assigned to me and both are actively being
> worked on, well one is depend on the other but still being worked on).

Ah, I see.

Paolo

Change target milestone to 4.2.3, as 4.2.2 has been released.

What is the status of this bug now?  Re. comment #39, a meta-bug for what? 
There is only one open bug left that depends on this one.

Are we still tracking an issue in this bug?  If so, what?  If not, please close
this bug report.

Hello world.  Please, a status update.

This has been fixed in mainline (4.3), but has not been fixed in 4.2.  I'm ok
with not back porting this to 4.2.  I'll give everyone a few days to object,
otherwise I'll change the Target Milestone to 4.3 and close as FIXED.

This is fixed on mainline and we're not going to backport it to 4.2, so I'm
changing the target milestone.

Author: bergner
Date: Mon Apr  7 17:36:59 2008
New Revision: 133985

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=133985
Log:
        PR middle-end/PR28690
        * rtlanal.c: Update copyright years.
        (commutative_operand_precedence): Give SYMBOL_REF's the same precedence
        as REG_POINTER and MEM_POINTER operands.
        * emit-rtl.c (gen_reg_rtx_and_attrs): New function.
        (set_reg_attrs_from_value): Call mark_reg_pointer as appropriate.
        * rtl.h (gen_reg_rtx_and_attrs): Add prototype for new function.
        * gcse.c: Update copyright years.
        (pre_delete): Call gen_reg_rtx_and_attrs.
        (hoist_code): Likewise.
        (build_store_vectors): Likewise.
        (delete_store): Likewise.
        * loop-invariant.c (move_invariant_reg): Likewise.
        Update copyright years.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/emit-rtl.c
    trunk/gcc/gcse.c
    trunk/gcc/loop-invariant.c
    trunk/gcc/rtl.h
    trunk/gcc/rtlanal.c

(In reply to comment #47)

>        * rtlanal.c: Update copyright years.
>        (commutative_operand_precedence): Give SYMBOL_REF's the same precedence

This change causes regression in i686-pc-linux-gnu testsuite:

FAIL: gcc.target/i386/addr-sel-1.c scan-assembler a\\+1
FAIL: gcc.target/i386/addr-sel-1.c scan-assembler b\\+1

Tracked in PR 35867.

The offending hunk has been reverted in revision 134095.

I guess that you had modified the precedences in order to allow additional
simplifications.  Can you report here what is missed using the current values?

Ok, I dug into this a little deeper.  For the following test case:

  int array[1024];
  void
  clear_table (unsigned int n)
  {
    unsigned int i;
    for (i = 0; i < n; i++)
      array[i] = 0;
  }

compiling this with -O1 (it's ok with -O2 or above) on powerpc{,64}-linux,
during expand, we call swap_commutative_operands_p with a SYMBOL_REF and a REG
which currently prefers the REG first.  Later, break_out_memory_refs forces the
SYMBOL_REF into a register (with the REG_POINTER attribute set), but we're
already done swapping, so we get the wrong operand ordering.  Paolo, I wonder
if this patch instead of the rtlanal.c hunk might be better.  It does fix my
problem:

Index: explow.c
===================================================================
--- explow.c    (revision 134095)
+++ explow.c    (working copy)
@@ -305,7 +305,7 @@ break_out_memory_refs (rtx x)
       rtx op1 = break_out_memory_refs (XEXP (x, 1));

       if (op0 != XEXP (x, 0) || op1 != XEXP (x, 1))
-       x = gen_rtx_fmt_ee (GET_CODE (x), Pmode, op0, op1);
+       x = simplify_gen_binary (GET_CODE (x), Pmode, op0, op1);
     }

   return x;

Subject: Re:  [4.2 Regression] Performace problem with
 indexed load/stores on powerpc


> Index: explow.c
> ===================================================================
> --- explow.c    (revision 134095)
> +++ explow.c    (working copy)
> @@ -305,7 +305,7 @@ break_out_memory_refs (rtx x)
>        rtx op1 = break_out_memory_refs (XEXP (x, 1));
> 
>        if (op0 != XEXP (x, 0) || op1 != XEXP (x, 1))
> -       x = gen_rtx_fmt_ee (GET_CODE (x), Pmode, op0, op1);
> +       x = simplify_gen_binary (GET_CODE (x), Pmode, op0, op1);
>      }

Definitely a good idea.

Author: bergner
Date: Wed Apr  9 13:42:43 2008
New Revision: 134139

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=134139
Log:
        PR middle-end/PR28690
        * explow.c (break_out_memory_refs): Use simplify_gen_binary rather
        than gen_rtx_fmt_ee to perform more canonicalizations.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/explow.c