Re: [avr-gcc-list] Bug in *rotlsi3 insns?

[Georg-Johann Lay]

Weddington, Eric schrieb:
>  
>
> > -----Original Message-----
> > From: 
> > address@hidden 
> > [mailto:address@hidden
> > org] On Behalf Of Georg-Johann Lay
> > Sent: Monday, March 23, 2009 1:49 PM
> > To: address@hidden
> > Subject: Re: [avr-gcc-list] Bug in *rotlsi3 insns?
> >
> > Georg-Johann Lay schrieb:
> > > Hi, im just browsing avr.md in trunk and stumbled over some 
> > new patterns 
> > > that implement rotlsi3.
> > >
> > > IMHO, they are buggy. But I have no testcase to make it explicit.
> > Consider this test case:
> >
> > unsigned long rotl (int dummy, unsigned long x)
> > {
> >      return (x << 8) | (x >> 24);
> > }
> >
> > Compile with, e.g.
> >     avr-gcc -mmcu=atmega8 -S -Os -fno-split-wide-types
> > to get
> >
> > rotl:
> > /* prologue: function */
> > /* frame size = 0 */
> >         mov r22,r23
> >         mov r23,r20
> >         mov r24,r21
> >         mov r25,r22
> > /* epilogue start */
> >         ret
> >
> > This will map 0x33221100 to 0x33110033 instead of to 0x22110033
>
> What do you get if you drop -fno-split-wide-types?

rotl:
/* prologue: function */
/* frame size = 0 */
        mov r18,r23      ;  34  *movqi/1        [length = 1]
        mov r19,r20      ;  35  *movqi/1        [length = 1]
        mov r20,r21      ;  36  *movqi/1        [length = 1]
        mov r21,r22      ;  37  *movqi/1        [length = 1]
        movw r22,r18     ;  43  *movhi/1        [length = 1]
        movw r24,r20     ;  42  *movhi/1        [length = 1]
/* epilogue start */
        ret      ;  40  return  [length = 1]

The code is correct in this context, but *rotlsi3_8 still occurs from 
combine to postreload. I think the bug is not triggered because the 
input is a pseudo, but what is generated looks not very healthy.

> And what version is this on?

I am playing on trunk, so it's not in the latest releases of avr-gcc:
   GNU C (GCC) version 4.4.0 20090323 (experimental) (avr)

The same bug should occur for similar rotate patterns.

BTW: Did you add -fno-split-wide-types to your testsuite options? IMHO 
it should be present for some runs together with -O[2s3]

Moreover, the following is incorrect:

(define_insn "*addhi3_zero_extend1"
  [(set (match_operand:HI 0 "register_operand" "=r")
        (plus:HI (match_operand:HI 1 "register_operand" "%0")
                 (zero_extend:HI
                  (match_operand:QI 2 "register_operand" "r"))))]

because #1 does not commute with #2 (reload will see #1 and #2, but not 
the zero_extend as the zero_extend is not part of the operand). The % 
should be removed. Either it is never used by reload for some reason, or 
it will shred some programs.

Then, RET_REGISTER is implemented as
int
avr_ret_register (void)
{
  return 24;
}

RET_REGISTER is outdated, the rubbish should be removed.

AFAIR this is now target hook TARGET_FUNCTION_VALUE, implemented in 
avr_function_value which looks ok at first sight. But

#define FUNCTION_VALUE_REGNO_P(N) ((int) (N) == RET_REGISTER)

does only mention R24 (R22, ... may also occur for SI/SF). This Macro is 
still active and used in middle-end.

Georg-Johann