[4.2] MIPS: missed optimization

[Richard Sandiford]

Manuel Lauss <mano@roarinelk.homelinux.net> writes:
> On Sun, Nov 23, 2008 at 05:14:27PM +0000, Richard Sandiford wrote:
>> Richard Sandiford <rdsandiford@googlemail.com> writes:
>> > Manuel Lauss <mano@roarinelk.homelinux.net> writes:
>> >> Admittedly my understanding of mips assembly is not yet very advanced, am I
>> >> missing something or is this a bug?
>> >
>> > Well, it's a missed optimisation, certainly.  Fortunately,
>> > it's conceptually fairly easy to fix.
>> 
>> FWIW, I've attached a patch against GCC 4.4 below.  Tested on
>> mipsisa64-elfoabi.
>> 
>> I don't think it's appropriate to apply the patch at this stage
>> in the development cycle, so I'll hold it back for GCC 4.5.
>> I'll add some testcases then too.
>
> I tested this with 4.3.2 and it works very well.

Great!  Thanks for testing it.

> One minor thing though if you don't mind.  Consider this C snippet:
>
> -------------- 8< ------------------------- >8 ----------------
>
> #define SYS_CNTRL_M21   (1 << 19)
> #define SYS_COUNTER_CNTRL 0xb1900014
> #define SYS_RTCMATCH2   0xb1900054
> #define SYS_RTCREAD     0xb1900058
>
> int au1x_rtcmatch2_set_next_event(unsigned long delta, void *x)
> {
>   unsigned long next;
>
>   next = *(unsigned long *)(SYS_RTCREAD) + delta;
>
>   /* wait for register access */
>   while (*(unsigned long *)(SYS_COUNTER_CNTRL) & SYS_CNTRL_M21)
>     ;
>   *(unsigned long *)(SYS_RTCMATCH2) = next;
>   asm volatile ("sync");
> }
>
> -------------- 8< ------------------------- >8 -------------
>
> My patched gcc generates this with -O2 (-Os is actually worse):
>
> 0x00000028 <au1x_rtcmatch2_set_next_event+0>:   lui     v0,0xb190
> 0x0000002c <au1x_rtcmatch2_set_next_event+4>:   lw      v1,88(v0)
> 0x00000030 <au1x_rtcmatch2_set_next_event+8>:   lui     a2,0xb190
> 0x00000034 <au1x_rtcmatch2_set_next_event+12>:  lui     a1,0x8
> 0x00000038 <au1x_rtcmatch2_set_next_event+16>:  lw      v0,20(a2)
> 0x0000003c <au1x_rtcmatch2_set_next_event+20>:  and     v0,v0,a1
> 0x00000040 <au1x_rtcmatch2_set_next_event+24>:  bnez    v0,0x38 <au1x_rtcmatch2_set_next_event+16>
> 0x00000044 <au1x_rtcmatch2_set_next_event+28>:  lui     v0,0xb190
> 0x00000048 <au1x_rtcmatch2_set_next_event+32>:  addu    v1,a0,v1
> 0x0000004c <au1x_rtcmatch2_set_next_event+36>:  sw      v1,84(v0)
> 0x00000050 <au1x_rtcmatch2_set_next_event+40>:  sync
> 0x00000054 <au1x_rtcmatch2_set_next_event+44>:  jr      ra
> 0x00000058 <au1x_rtcmatch2_set_next_event+48>:  nop
>
> IMO gcc's register usage is a bit odd;  when the 'addu' at 0x48
> is moved to 0x30 (as 'addu a0,a0,v1'), v1 could be reused and the
> two additional 'lui v0,0xb190' become unecessary, saving 2 insns.
> (This code is called fairly often, so I do care about performance ;-))

Hmm, GCC 4.4 suffers from the same problem.  Before CSE1 we have:

     R1 := 0xb1900000
     load R1+88
     ...
loop:
     ...
     R2 := 0xb1900000
     load R2+20
     ...
     R3 := 0xb1900000
     store R3+84

After CSE we have:

     R1 := 0xb1900000
     load R1+88
     ...
loop:
     ...
     R2 := 0xb1900000
     load R2+20
     ...
     store R2+84           <--- A

where the important thing is that A uses R2 instead of R1.
Loop hoisting converts this to:

     R1 := 0xb1900000
     load R1+88
     ...
     R4 := 0xb1900000
loop:
     ...
     R2 := R4
     load R4+20
     ...
     store R2+84

where the loop still has an assigment to R2, even though it is
now only used after the loop.  Then the GCSE "bypass" pass comes
along and replaces this with:

     R1 := 0xb1900000
     load R1+88
     ...
     R4 := 0xb1900000
loop:
     ...
     R2 := 0xb1900000
     load R4+20
     ...
     store R2+84           <--- A

As a workaround, -fno-gcse produces good code for this testcase with 4.4.
Maybe it will for 4.3 too.  It might well make other functions worse though.

Richard