[Bug rtl-optimization/49807] New: Missed byte (subreg) extraction

[Bug rtl-optimization/49807] New: Missed byte (subreg) extraction

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49807

           Summary: Missed byte (subreg) extraction
           Product: gcc
           Version: 4.6.1
            Status: UNCONFIRMED
          Keywords: missed-optimization
          Severity: normal
          Priority: P3
         Component: rtl-optimization
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: gjl@gcc.gnu.org
            Target: avr


This C source:

#define SPDR (*((char volatile*) 0x2c))

void read_adc (long big)
{ 
   SPDR = big >> 24;
   SPDR = big >> 16;
   SPDR = big >> 8; 
   SPDR = big;
} 

compiles with 
   avr-gcc -S -Os -dp -mmcu=atmega8
to:

read_adc:
    movw r26,r24     ;  2    *movsi/1    [length = 2]
    movw r24,r22
    mov r20,r27     ;  28    *ashrsi3_const/3    [length = 6]
    clr r23
    sbrc r20,7
    com r23
    mov r21,r23
    mov r22,r23
    out 44-0x20,r20     ;  9    *movqi/3    [length = 1]
    movw r20,r26     ;  29    *ashrsi3_const/3    [length = 5]
    clr r23
    sbrc r21,7
    com r23
    mov r22,r23
    out 44-0x20,r20     ;  13    *movqi/3    [length = 1]
    mov r20,r25     ;  30    *ashrsi3_const/3    [length = 6]
    mov r21,r26
    mov r22,r27
    clr r23
    sbrc r22,7
    dec r23
    out 44-0x20,r20     ;  17    *movqi/3    [length = 1]
    out 44-0x20,r24     ;  20    *movqi/3    [length = 1]
    ret     ;  26    return    [length = 1]

The shifts are done explicitely where the bytes could be saved directly.

Combiner tries insns like

Failed to match this instruction:
(set (mem/v:QI (const_int 44))
     (subreg:QI (reg:SI 49) 1))

But they don't match because of MEM_VOLATILE_P so that the MEM
does not match memory_operand as obviously volatile_ok is 0 at that moment.

Must the backend split such insns by hand?

Changing the source like

#define SPDR0 (*((char*) 0x2c))
#define SPDR1 (*((char*) 0x2d))
#define SPDR2 (*((char*) 0x2e))
#define SPDR3 (*((char*) 0x2f))

void read_adc (long big)
{ 
   SPDR0 = big >> 24;
   SPDR1 = big >> 16;
   SPDR2 = big >> 8;
   SPDR3 = big;
}


and it compiles fine:

read_adc:
    out 44-0x20,r25     ;  8    *movqi/3    [length = 1]
    out 45-0x20,r24     ;  11    *movqi/3    [length = 1]
    out 46-0x20,r23     ;  14    *movqi/3    [length = 1]
    out 47-0x20,r22     ;  16    *movqi/3    [length = 1]
    ret     ;  26    return    [length = 1]


== configure ==

Target: avr
Configured with: ../../gcc.gnu.org/gcc-4_6-branch/configure --target=avr
--prefix=/local/gnu/install/gcc-4.6-mingw32 --host=i586-mingw32
--build=i686-linux-gnu --enable-languages=c,c++ --disable-nls --disable-shared
--with-dwarf2
Thread model: single
gcc version 4.6.1 20110620 (prerelease) (GCC)

[Bug rtl-optimization/49807] Missed byte (subreg) extraction

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49807

--- Comment #1 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2011-07-21 19:34:27 UTC ---
Created attachment 24804
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24804
C test case as in the initial PR

[Bug rtl-optimization/49807] Missed byte (subreg) extraction

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49807

Georg-Johann Lay <gjl at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |eric.weddington at atmel
                   |                            |dot com
   Target Milestone|---                         |4.7.0

[Bug rtl-optimization/49807] Missed byte (subreg) extraction when storing to volatile mem

[eric.weddington at atmel dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49807

--- Comment #2 from Eric Weddington <eric.weddington at atmel dot com> 2011-07-21 20:12:58 UTC ---
(In reply to comment #0)
> This C source:
> 
> #define SPDR (*((char volatile*) 0x2c))

Hi Johann,

That's not quite correct. In avr-libc the header file sfr_defs.h will define a
register as this:

#define SPDR (*((volatile char *) 0x2c))

It's a pointer to a volatile char, not a volatile pointer to a char.

> Changing the source like
> 
> #define SPDR0 (*((char*) 0x2c))
> #define SPDR1 (*((char*) 0x2d))
> #define SPDR2 (*((char*) 0x2e))
> #define SPDR3 (*((char*) 0x2f))
> 
> void read_adc (long big)
> { 
>    SPDR0 = big >> 24;
>    SPDR1 = big >> 16;
>    SPDR2 = big >> 8;
>    SPDR3 = big;
> }
> 
> 
> and it compiles fine:

Is your intent to change the address AND remove the volatile keyword? If you
want to test if it's the volatile keyword that is the cause then you should
only change that part:

#define SPDR (*((char *) 0x2c))

void read_adc (long big)
{ 
   SPDR = big >> 24;
   SPDR = big >> 16;
   SPDR = big >> 8;
   SPDR = big;
}


Overall, though, I think you're on the right track. Most users would like to be
able to do the shift-and-assign pattern in C and have it compile to storing the
individual byte without a shift in the assembly. Right now, the only way to do
a workaround to achieve that result is through the use of a union with a struct
and an integer type (like a long). If this issue can be fixed then I think that
this has a chance to reduce a lot of code size problems.

[Bug rtl-optimization/49807] Missed byte (subreg) extraction when storing to volatile mem

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49807

--- Comment #3 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2011-07-21 20:34:14 UTC ---
> (In reply to comment #2)
> That's not quite correct. In avr-libc the header file sfr_defs.h will define a
> register as this:
> 
> #define SPDR (*((volatile char *) 0x2c))

(volatile char *) is the same as (char volatile *). Don't confuse it with (char
* volatile) etc.

> Is your intent to change the address AND remove the volatile keyword?

I changed to different addresses in the non-volatile example because otherwise
just the last store will survive.

> Overall, though, I think you're on the right track. Most users would like to be
> able to do the shift-and-assign pattern in C and have it compile to storing the
> individual byte without a shift in the assembly. Right now, the only way to do
> a workaround to achieve that result is through the use of a union with a struct
> and an integer type (like a long). If this issue can be fixed then I think that
> this has a chance to reduce a lot of code size problems.

I don't know the exact rationale why volatile_ok is false in combine.
It' obviously about volatile correctnet, but I don't see what would break here.

Re: [Bug rtl-optimization/49807] Missed byte (subreg) extraction when storing to volatile mem

[Andrew Pinski]

On Thu, Jul 21, 2011 at 1:34 PM, gjl at gcc dot gnu.org
<gcc-bugzilla@gcc.gnu.org> wrote:
> I don't know the exact rationale why volatile_ok is false in combine.
> It' obviously about volatile correctnet, but I don't see what would break here.

It can, when dealing optimizations that reduce the size of load/stores
of volatile memory.

Thanks,
Andrew Pinski

[Bug rtl-optimization/49807] Missed byte (subreg) extraction when storing to volatile mem

[pinskia at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49807

--- Comment #4 from pinskia at gmail dot com <pinskia at gmail dot com> 2011-07-21 20:39:08 UTC ---
On Thu, Jul 21, 2011 at 1:34 PM, gjl at gcc dot gnu.org
<gcc-bugzilla@gcc.gnu.org> wrote:
> I don't know the exact rationale why volatile_ok is false in combine.
> It' obviously about volatile correctnet, but I don't see what would break here.

It can, when dealing optimizations that reduce the size of load/stores
of volatile memory.

Thanks,
Andrew Pinski

[Bug rtl-optimization/49807] Missed byte (subreg) extraction when storing to volatile mem

[gjl at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49807

--- Comment #5 from Georg-Johann Lay <gjl at gcc dot gnu.org> 2011-07-21 20:46:05 UTC ---
In this case it would not reduce the size of the store, but how can this be
seen?
It cannot be seen from the insn/pattern alone.

Do you have an idea how to attack this optimization flaw?

[Bug rtl-optimization/49807] Missed byte (subreg) extraction when storing to volatile mem

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49807

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Target|avr                         |avr, mips*-*-*
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011.07.23 22:15:10
   Target Milestone|4.7.0                       |---
     Ever Confirmed|0                           |1

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> 2011-07-23 22:15:10 UTC ---
I saw this on MIPS too.(In reply to comment #5)
> Do you have an idea how to attack this optimization flaw?
To figure out the places which reduce the MEM's sizes and conditionalize them
on non volatile MEM's.
And then remove the condition in combine for not allowing volatile memory
optimizations.

[Bug rtl-optimization/49807] Missed byte (subreg) extraction when storing to volatile mem

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49807

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement