Re: GCC 3.1 Issues

[Daniel Berlin <dan at dberlin dot org>]

On Sun, 10 Mar 2002, Andreas Jaeger wrote:

> Daniel Berlin <dan@dberlin.org> writes:
> 
> > On Sun, 2002-03-10 at 08:29, Roger Sayle wrote:
> >> 
> >> Hi Andreas,
> >> > I'm a little bit puzzled.
> >> > But the inline function does not "modify memory in an unpredictable
> >> > fashion", we just access it.
> >> >
> >> > So, my questions are:
> >> > - is the inline function correct?
> >> > - is the manual correct?
> >> 
> >> That was my first take on the problem which was why I submitted the
> >> patch.  However, Richard's suggestion of adding a "memory" clobber
> >> does indeed fix the problem (both with your original __asm__ and my
> >> distilled test case).
> >> 
> >> If rth's interpretation is right, the manual needs to be corrected.
> >> If the manual is right, someone should consider applying my patch.
> >> In the worst case my patch only disables "store motion" across __asm__
> >> statements, rather than disabling them entirely.
> >> 
> >> 
> >> As the SPEC2000 guy :>, could you check how much store motion actually
> >> effects performance?  Seems silly to argue over a possibly unmeasurable
> >> optimization.
> >
> > Um, right now?
> > It won't make a measurable difference, since it'll never think stores
> > are okay when they are.
> > It also won't hoist anything loop already doesn't handle.
> 
> Here're the requested;-) numbers for some SPEC benchmarks with -O3
> -march=athlon-xp:
> 
> with store motion:
>  Success 164.gzip ratio=629.90, runtime=222.257335
>  Success 164.gzip ratio=630.69, runtime=221.978712
>  Success 164.gzip ratio=630.95, runtime=221.888577
>  Success 175.vpr ratio=322.27, runtime=434.413673
>  Success 175.vpr ratio=322.41, runtime=434.234616
>  Success 175.vpr ratio=322.22, runtime=434.484413
> 
> without store motion:
>  Success 164.gzip ratio=630.61, runtime=222.008486
>  Success 164.gzip ratio=630.86, runtime=221.919070
>  Success 164.gzip ratio=630.82, runtime=221.932230
>  Success 175.vpr ratio=322.91, runtime=433.553449
>  Success 175.vpr ratio=322.38, runtime=434.271049
>  Success 175.vpr ratio=322.84, runtime=433.654110
> 
> So no real difference for these two tests.  I'll do a full spec run
> tomorrow on another machine.

As I said, it's pointless.
store_ops_ok *can't* return 1 when a store is okay to move, because of the 
reversed test.
The only stores it would return okay for, are stores that are killed, and 
since they got eliminated elsewhere, ...

Even if it did return the right value, the conditions necessary for it to 
consider it a moveable store will prevent anything that loop can't move 
from being considered, unless it hits some very odd corner case or 
somethign.

The only stores store motion currently *could* move (if the test in 
store_ops_ok reutrned the right value) would be stores that met *all* of 
the following conditions

1. It's a set without asm operands
2. Non-volatile, not blockmode
3. Has to store into a symbol ref
4. rtx_varies_p (x) == 0


Note that #3 eliminates a lot of stores
4 eliminates anything loop can't move anyway, except for cases where loop 
isn't doing what it should be doing.

I'm all for having store motion (i wouldn't have spent months 
trying to fix it otherwise), but what we have now is worthless.
here's a simple patch to make the test go in the right direction (IE 
store_ops_ok will return 1 if it's okay, rather than returning 0).
See if it still fails make check on x86, and if not, see if it has *any* 
value at all.
I doubt it, but it's a start until i get the store motion patches back 
into shape.
Index: gcse.c
===================================================================
RCS file: /cvs/gcc/egcs/gcc/gcse.c,v
retrieving revision 1.180
diff -c -3 -p -w -B -b -r1.180 gcse.c
*** gcse.c      2002/03/08 20:23:55     1.180
--- gcse.c      2002/03/10 21:39:13
*************** store_ops_ok (x, bb)
*** 6308,6314 ****
      case REG:
        /* If a reg has changed after us in this
           block, the operand has been killed.  */
!       return TEST_BIT (reg_set_in_block[bb->index], REGNO (x));

      case MEM:
        x = XEXP (x, 0);
--- 6308,6314 ----
      case REG:
        /* If a reg has changed after us in this
           block, the operand has been killed.  */
!       return !TEST_BIT (reg_set_in_block[bb->index], REGNO (x));

      case MEM:
        x = XEXP (x, 0);

> Send it to the list and I - and probably others - will give it again
> some testing and hopefully others will help fixing that aliasing bug
> if it still exists,
Okay, give me a few days to remake diffs. I only have the gcse.c from the 
store motion work, not actual code anymore.

> 
> Thanks,
> Andreas
>