[Bug middle-end/42722] move_by_pieces() incorrectly pushes structures to stack

[Bug middle-end/42722] move_by_pieces() incorrectly pushes structures to stack

[bigotp at acm dot org]

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

--- Comment #12 from Peter A. Bigot <bigotp at acm dot org> 2011-02-21 09:45:15 UTC ---
Created attachment 23418
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=23418
Test case (requires msp430 back end)

Sorry, I don't speak dejagnu well enough yet to put target test conditionals
into the code; this test lives in the gcc.target/msp430 directory in my
environment.

[Bug middle-end/42722] move_by_pieces() incorrectly pushes structures to stack

[bigotp at acm dot org]

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

--- Comment #14 from Peter A. Bigot <bigotp at acm dot org> 2011-02-21 09:46:30 UTC ---
Created attachment 23420
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=23420
Generated code after patch applied

[Bug middle-end/42722] move_by_pieces() incorrectly pushes structures to stack

[bigotp at acm dot org]

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

--- Comment #13 from Peter A. Bigot <bigotp at acm dot org> 2011-02-21 09:45:52 UTC ---
Created attachment 23419
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=23419
Generated code before patch applied

[Bug middle-end/42722] move_by_pieces() incorrectly pushes structures to stack

[bigotp at acm dot org]

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

--- Comment #15 from Peter A. Bigot <bigotp at acm dot org> 2011-02-21 09:48:23 UTC ---
Created attachment 23421
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=23421
Fixes problem

[Bug middle-end/42722] move_by_pieces() incorrectly pushes structures to stack

[bigotp at acm dot org]

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

--- Comment #16 from Peter A. Bigot <bigotp at acm dot org> 2011-02-21 09:50:00 UTC ---
I've now taken over the msp430 back end and created the attached patch and test
case.  This is relative to the gcc trunk as of a couple weeks ago; still
present in all releases up to that point.

Sorry it doesn't show up with any other back end, but perhaps an appeal to
inspection could work while we get the msp430 target ready for integration: 
This is the only conditional of four in the same block that does not check for
data.reverse before doing its thing.

It's definitely wrong.

[Bug middle-end/42722] move_by_pieces() incorrectly pushes structures to stack

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42722

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|major                       |enhancement
           Keywords|                            |missed-optimization

[Bug middle-end/42722] move_by_pieces() incorrectly pushes structures to stack

[shcherbakov at daad-alumni dot de]

------- Comment #11 from shcherbakov at daad-alumni dot de  2010-02-10 12:44 -------
Ok, m32c uses "memcpy" for all block moves (4 ints is a DImode, not BLKmode).
Additionally, it supports pre-decrement addressing mode, that enables a
mutually exclusive case to the one showing the bug.
To reproduce the bug on m32c, change the following:
1. in m32c.h replace "#define HAVE_PRE_DECREMENT 1" with 0
2. Comment out "(define_expand "movmemhi"" in blkmov.md
Then, compiling the example with structure containing 5 integers produces this:
        .file   "0.c"
.text
        .global _func1
        .type   _func1, @function
_func1:
        enter   #0
        push.w  7[fb]
        push.w  5[fb]
        mova    9[fb],a0
        push.w  2[a0]
        push.w  [a0]
        push.w  4[a0]
        jsr.a   _func2
        add.w   #10,sp
        exitd
        .size   _func1, .-_func1
        .ident  "GCC: (GNU) 4.4.3"

Obviously, the push order (fp+7,fp+5,fp+11,fp+9,...) is incorrect.

Note that removing hardcoded "memcpy" call and commenting out pre-increment
support was needed ONLY to reproduce the bug normally happening on MSP430 using
a supported M32C platform.


-- 


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

[Bug middle-end/42722] move_by_pieces() incorrectly pushes structures to stack

[mikpe at it dot uu dot se]

------- Comment #10 from mikpe at it dot uu dot se  2010-02-09 08:58 -------
(In reply to comment #9)
> Finally, the m32c port defines both PUSH_ARGS and HAVE_POST_INCREMENT, so it
> can be possible to reproduce the bug on this port.

> cat pr42722.c
struct test {
    int a, b, c, d;
};
void func2(struct test copy);
void func1(struct test copy) {func2(copy);}
> m32c-unknown-elf-gcc -Os -S pr42722.c
> cat pr42722.s 
        .file   "pr42722.c"
.text
        .global _func1
        .type   _func1, @function
_func1:
        enter   #0
        push.w  11[fb]
        push.w  9[fb]
        push.w  7[fb]
        push.w  5[fb]
        jsr.a   _func2
        add.w   #8,sp
        exitd
        .size   _func1, .-_func1
        .ident  "GCC: (GNU) 4.4.3"

which looks correct to me (not knowing m32c asm at all). With a larger struct
gcc generates a memcpy() call instead.


-- 


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

[Bug middle-end/42722] move_by_pieces() incorrectly pushes structures to stack

[shcherbakov at daad-alumni dot de]

------- Comment #9 from shcherbakov at daad-alumni dot de  2010-01-30 22:48 -------
In general 'black box' case - yes. However, here there are 2 factors:
1) It is clear from the sources that 2 similar situations (pre-decrement and
post-increment) are handled in a different way, an additional check performed
in pre-decrement is forgotten in post-increment mode.
2) The bug is clearly reproduced on the non-standard msp430-gcc target.
However, the analysis of the expr.c code (see factor 1) indicates that this is
a general GCC problem caused by rarely occurring factor combination, rather
than bug in msp430 target implementation. To reproduce it outside this target,
you need to combine 2 factors: supported USE_LOAD_POST_INCREMENT and supported
PUSH_ARGS.
You can alternatively build the msp430-gcc compiler (see
http://mspgcc4.sourceforge.net/, the site provides an automatic downloading &
building script) to see a non-broken gcc compiler having the problem (you will
need to undo the fix applied by MSPGCC4 build script).

The problem is that "a case not used by mainstream ports is handled incorrectly
by platform-independent part of GCC" and the problem now is to prove that the
bug is common to gcc and not caused by msp430 port. Running a modified ARM port
in debugger (setting breakpoint on mve_by_pieces()) can clearly illustrate, how
GCC makes wrong decision to push the structure in a reversed way. The code
making this decision (and generating move insns) does not depend on any other
global definitions and is not related to the problem in __muldi3().

Finally, the m32c port defines both PUSH_ARGS and HAVE_POST_INCREMENT, so it
can be possible to reproduce the bug on this port.


-- 


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

[Bug middle-end/42722] move_by_pieces() incorrectly pushes structures to stack

[mikpe at it dot uu dot se]

------- Comment #8 from mikpe at it dot uu dot se  2010-01-30 21:57 -------
(In reply to comment #7)
> (In reply to comment #6)
> > Doesn't work, an ARM gcc built with that modification ICEs while trying to
> > compile __muldi3() in libgcc2.c.
> You don't need libgcc to reproduce the problem. If gcc build has gone to this
> step, the xgcc/cc1/cc1plus were successfully built and can be called as
> "<your-gcc-build-directory>/gcc/xgcc". Just invoke cc1plus from the build/gcc
> directory directly:
> ./cc1plus -O2 test.cpp -o test.S
> The resulting .S file should be enough to see whether the problem is reproduced
> on ARM.

A known broken gcc cannot be used to prove or disprove a bug in gcc.


-- 

mikpe at it dot uu dot se changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|mikpe at it dot uu dot se   |


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

[Bug middle-end/42722] move_by_pieces() incorrectly pushes structures to stack

[shcherbakov at daad-alumni dot de]

------- Comment #7 from shcherbakov at daad-alumni dot de  2010-01-30 21:30 -------
(In reply to comment #6)
> Doesn't work, an ARM gcc built with that modification ICEs while trying to
> compile __muldi3() in libgcc2.c.
You don't need libgcc to reproduce the problem. If gcc build has gone to this
step, the xgcc/cc1/cc1plus were successfully built and can be called as
"<your-gcc-build-directory>/gcc/xgcc". Just invoke cc1plus from the build/gcc
directory directly:
./cc1plus -O2 test.cpp -o test.S
The resulting .S file should be enough to see whether the problem is reproduced
on ARM.


-- 


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

[Bug middle-end/42722] move_by_pieces() incorrectly pushes structures to stack

[mikpe at it dot uu dot se]

------- Comment #6 from mikpe at it dot uu dot se  2010-01-30 21:24 -------
(In reply to comment #5)
> ARM uses ACCUMULATE_OUTGOING_ARGS instead of PUSH_ARGS. To reproduce the
> problem on ARM, you need to comment out "#define ACCUMULATE_OUTGOING_ARGS 1"
> and add "#define PUSH_ARGS 1" in "gcc/config/arm/arm.h"

Doesn't work, an ARM gcc built with that modification ICEs while trying to
compile __muldi3() in libgcc2.c.


-- 


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

[Bug middle-end/42722] move_by_pieces() incorrectly pushes structures to stack

[shcherbakov at daad-alumni dot de]

------- Comment #5 from shcherbakov at daad-alumni dot de  2010-01-30 20:08 -------
Oops, missed a semicolon in the first line of "test case" (compile it with
g++).
ARM uses ACCUMULATE_OUTGOING_ARGS instead of PUSH_ARGS. To reproduce the
problem on ARM, you need to comment out "#define ACCUMULATE_OUTGOING_ARGS 1"
and add "#define PUSH_ARGS 1" in "gcc/config/arm/arm.h", so that func1() will
contain several "push" instructions rather than "mov xxx, [sp + yyy]".
To see that the code has been compiled incorrectly, simply examine the assembly
output of "func1()", that will push the "copy" to stack starting with first
word and ending with the last one (causing the layout in stack to be inverted).


-- 


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

[Bug middle-end/42722] move_by_pieces() incorrectly pushes structures to stack

[mikpe at it dot uu dot se]

------- Comment #4 from mikpe at it dot uu dot se  2010-01-30 19:54 -------
(In reply to comment #2)
> However, as shown in the patch, the discovered problem is general to GCC4 (will
> cause problems on all targets having USE_LOAD_PRE_DECREMENT)

The "test case" is syntactically invalid and doesn't do anything to inform the
user if it's been miscompiled. Fixing those issues I still don't see anything
wrong on ARM (which defines USE_LOAD_PRE_DECREMENT via HAVE_PRE_DECREMENT).


-- 

mikpe at it dot uu dot se changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mikpe at it dot uu dot se


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

[Bug middle-end/42722] move_by_pieces() incorrectly pushes structures to stack

[rguenth at gcc dot gnu dot org]

------- Comment #3 from rguenth at gcc dot gnu dot org  2010-01-30 16:01 -------
Patches should be sent to gcc-patches@gcc.gu.org with a ChangeLog entry and
a note how you tested the patch.


-- 


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

[Bug middle-end/42722] move_by_pieces() incorrectly pushes structures to stack

[shcherbakov at daad-alumni dot de]

------- Comment #2 from shcherbakov at daad-alumni dot de  2010-01-30 15:37 -------
(In reply to comment #1)
> What is msp430-gcc?
> 

msp430-gcc is a port of GCC on TI MSP430 platform, that revealed the bug.
However, as shown in the patch, the discovered problem is general to GCC4 (will
cause problems on all targets having USE_LOAD_PRE_DECREMENT) and is not
specific to msp430-gcc.


-- 


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

[Bug middle-end/42722] move_by_pieces() incorrectly pushes structures to stack

[steven at gcc dot gnu dot org]

------- Comment #1 from steven at gcc dot gnu dot org  2010-01-30 15:33 -------
What is msp430-gcc?


-- 


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