* Testing m68k changes on AmigaOS and Linux/m68k @ 2003-10-12 4:07 Bernardo Innocenti 2003-10-13 17:24 ` Gunther Nikl 2003-10-31 23:47 ` Matthias Klose 0 siblings, 2 replies; 20+ messages in thread From: Bernardo Innocenti @ 2003-10-12 4:07 UTC (permalink / raw) To: Gunther Nikl; +Cc: GCC Mailing List Hello Gunther, I've recently applied all my remaining m68k patches. Since I can only test on ColdFire targets, would you mind doing a bootstrap on Linux/m68k and/or AmigaOS? -- // Bernardo Innocenti - Develer S.r.l., R&D dept. \X/ http://www.develer.com/ Please don't send Word attachments - http://www.gnu.org/philosophy/no-word-attachments.html ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Testing m68k changes on AmigaOS and Linux/m68k 2003-10-12 4:07 Testing m68k changes on AmigaOS and Linux/m68k Bernardo Innocenti @ 2003-10-13 17:24 ` Gunther Nikl 2003-10-14 12:40 ` Bernardo Innocenti 2003-10-31 23:47 ` Matthias Klose 1 sibling, 1 reply; 20+ messages in thread From: Gunther Nikl @ 2003-10-13 17:24 UTC (permalink / raw) To: Bernardo Innocenti; +Cc: GCC Mailing List Hello Bernardo, > I've recently applied all my remaining m68k patches. Your changes caused some breakage for my patches ;-) > Since I can only test on ColdFire targets, would you mind doing a > bootstrap on Linux/m68k and/or AmigaOS? I maintain my patches on a cross-setup by building a cross-compiler. That cross-compiler is then used to build a "native" compiler for my target. I don't boostrap on the target because thats much to slow for my taste ;-( Building the cross-compiler revealed two errors in m68k.c. The patch is attached. Using the cross-compiler to built the native compiler revealed two bugs (?) in gcc/Makefile[.in]. I changed the Makefile and was able to build the native compiler but I don't know whether my changes to the Makefile are correct. The cross-compiler to AmigaOS seems to work correctly (modulo the open issues regarding small-data). I goint to test the coss-built native compiler tonight. Gunther --cut-- 2003-10-13 Gunther Nikl <gni@gecko.de> * config/m68k/m68k.c (m68k_output_function_prologue): Fix usage of current_frame at two places (one type and one missing) --- m68k.c.orig Mon Oct 13 11:35:20 2003 +++ m68k.c Mon Oct 13 14:48:23 2003 @@ -623,7 +623,7 @@ #ifdef MOTOROLA asm_fprintf (stream, "\tfmovm %I0x%x,-(%Rsp)\n", current_frame.fpu_mask); #else - asm_fprintf (stream, "\tfmovem %I0x%x,%Rsp@-\n", current_frmae.fpu_mask); + asm_fprintf (stream, "\tfmovem %I0x%x,%Rsp@-\n", current_frame.fpu_mask); #endif if (dwarf2out_do_frame ()) { @@ -934,7 +934,7 @@ #else asm_fprintf (stream, "\tmoveml %s@(-%wd),%I0x%x\n", reg_names[FRAME_POINTER_REGNUM], - offset + fsize, + current_frame.offset + fsize, current_frame.reg_mask); #endif } --cut-- ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Testing m68k changes on AmigaOS and Linux/m68k 2003-10-13 17:24 ` Gunther Nikl @ 2003-10-14 12:40 ` Bernardo Innocenti 2003-10-14 13:56 ` Gunther Nikl 0 siblings, 1 reply; 20+ messages in thread From: Bernardo Innocenti @ 2003-10-14 12:40 UTC (permalink / raw) To: Gunther Nikl; +Cc: GCC Mailing List, Richard Henderson Gunther Nikl wrote: >>Since I can only test on ColdFire targets, would you mind doing a >>bootstrap on Linux/m68k and/or AmigaOS? > > I maintain my patches on a cross-setup by building a cross-compiler. > That cross-compiler is then used to build a "native" compiler for my > target. I don't boostrap on the target because thats much to slow for > my taste ;-( I guess building on my old Amiga 4000 with its 25MHz 68040 would take a couple of days :-) > Building the cross-compiler revealed two errors in m68k.c. The patch is > attached. Using the cross-compiler to built the native compiler revealed > two bugs (?) in gcc/Makefile[.in]. You're probably one of the few people who exercise canadian cross builds :-) > I changed the Makefile and was able > to build the native compiler but I don't know whether my changes to the > Makefile are correct. The cross-compiler to AmigaOS seems to work > correctly (modulo the open issues regarding small-data). I goint to test > the coss-built native compiler tonight. I see you're using the MIT syntax on the Amiga. Some guy told me the GeekGadgets port of GCC used it. Have you ever tried defining MOTOROLA? If you're luckly, it should work out of the box. I'm asking because I still have a (not so hidden) agenda for obsoleting the MIT syntax some day... Your patch looks fine to me, but it doesn't fall under the "obvious patch" definition (it changes code). Richard, is it OK to apply? > --cut-- > 2003-10-13 Gunther Nikl <gni@gecko.de> > > * config/m68k/m68k.c (m68k_output_function_prologue): Fix usage of > current_frame at two places (one type and one missing) > > --- m68k.c.orig Mon Oct 13 11:35:20 2003 > +++ m68k.c Mon Oct 13 14:48:23 2003 > @@ -623,7 +623,7 @@ > #ifdef MOTOROLA > asm_fprintf (stream, "\tfmovm %I0x%x,-(%Rsp)\n", current_frame.fpu_mask); > #else > - asm_fprintf (stream, "\tfmovem %I0x%x,%Rsp@-\n", current_frmae.fpu_mask); > + asm_fprintf (stream, "\tfmovem %I0x%x,%Rsp@-\n", current_frame.fpu_mask); > #endif > if (dwarf2out_do_frame ()) > { > @@ -934,7 +934,7 @@ > #else > asm_fprintf (stream, "\tmoveml %s@(-%wd),%I0x%x\n", > reg_names[FRAME_POINTER_REGNUM], > - offset + fsize, > + current_frame.offset + fsize, > current_frame.reg_mask); > #endif > } > --cut-- -- // Bernardo Innocenti - Develer S.r.l., R&D dept. \X/ http://www.develer.com/ Please don't send Word attachments - http://www.gnu.org/philosophy/no-word-attachments.html ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Testing m68k changes on AmigaOS and Linux/m68k 2003-10-14 12:40 ` Bernardo Innocenti @ 2003-10-14 13:56 ` Gunther Nikl 2003-10-14 17:00 ` Bernardo Innocenti 0 siblings, 1 reply; 20+ messages in thread From: Gunther Nikl @ 2003-10-14 13:56 UTC (permalink / raw) To: Bernardo Innocenti; +Cc: GCC Mailing List, Richard Henderson On Tue, Oct 14, 2003 at 10:07:44AM +0200, Bernardo Innocenti wrote: > You're probably one of the few people who exercise canadian cross > builds :-) Quite possible. I started doing such builds this year when I discovered how to do that and was surprised to see how "easy" it is :-) > I see you're using the MIT syntax on the Amiga. Some guy told me the > GeekGadgets port of GCC used it. You have a really bad memory ;-) I told you that. > Have you ever tried defining MOTOROLA? If you're luckly, it should work > out of the box. I never tried and I won't try it because the assembler I am using doesn't support it. > I'm asking because I still have a (not so hidden) agenda for obsoleting > the MIT syntax some day... I know and I am still against it. Such bugs could be caught easily by converting from #if[n]def MOTOROLA to if (MOTOROLA) with MOTOROLA defined to 0 or 1. Then the compiler would eliminate the dead code. > > I going to test the coss-built native compiler tonight. Unfortunately the cross-built compiler doesn't work :-( It seems to be miscompiled since I get this error message: ./cc1 -E foo.c <internal>:0: internal compiler error: tree check: expected class 'd', have 'd' (function_decl) in make_decl_rtl, at varasm.c:882 The compiler was built by a cross-compiler built from the same source as this one. A native 3.4 built at the beginning of September 2003 works. I am going to test whether 3.3 and an older 3.4 as build compiler will do better. Currently I don't know when it broke :-/ Gunther ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Testing m68k changes on AmigaOS and Linux/m68k 2003-10-14 13:56 ` Gunther Nikl @ 2003-10-14 17:00 ` Bernardo Innocenti 2003-10-15 12:40 ` Gunther Nikl 2003-10-15 13:57 ` Gunther Nikl 0 siblings, 2 replies; 20+ messages in thread From: Bernardo Innocenti @ 2003-10-14 17:00 UTC (permalink / raw) To: Gunther Nikl; +Cc: GCC Mailing List, Richard Henderson Gunther Nikl wrote: > On Tue, Oct 14, 2003 at 10:07:44AM +0200, Bernardo Innocenti wrote: > >>You're probably one of the few people who exercise canadian cross >>builds :-) > > Quite possible. I started doing such builds this year when I discovered > how to do that and was surprised to see how "easy" it is :-) I've played with the "combined tree" build instead. It's also much easier than it may seem from the instructions. Basically, I've checked out both the gcc and src repositories and combine them with a simple script: VERS=HEAD DIR=combined-$VERS rm -rf $DIR mkdir $DIR cd src-$VERS && find . -print | cpio -pdlm ../$DIR && cd .. cd gcc-$VERS && find . -print | cpio -pdlmu ../$DIR && cd .. Then I can build cross-compilers for several architectures with scripts like this: ROOTDIR=`pwd` ARCH=arm-elf VERS=HEAD BUILDDIR=${ARCH}-${VERS}-build INSTALLDIR=${ARCH}-${VERS}-install export CFLAGS= export CXXFLAGS= rm -rf $BUILDDIR $INSTALLDIR mkdir -p $BUILDDIR $INSTALLDIR cd $BUILDDIR ../combined-${VERS}/configure --target=$ARCH --prefix=${ROOTDIR}/${INSTALLDIR} --with-newlib --disable-gdbtk --enable-languages='c,c++' make >>I see you're using the MIT syntax on the Amiga. Some guy told me the >>GeekGadgets port of GCC used it. > > You have a really bad memory ;-) I told you that. Yes I do... Argh :-) >>Have you ever tried defining MOTOROLA? If you're luckly, it should work >>out of the box. > > I never tried and I won't try it because the assembler I am using > doesn't support it. I seem to recall you told me that too, but I can't remember why. Can't you use gas? >>I'm asking because I still have a (not so hidden) agenda for obsoleting >>the MIT syntax some day... > > I know and I am still against it. Such bugs could be caught easily by > converting from #if[n]def MOTOROLA to if (MOTOROLA) with MOTOROLA defined > to 0 or 1. Then the compiler would eliminate the dead code. It's not just a testing issue. Keeping both paths in sync is tedious work and makes reading the code much harder. Anyway, since the MIT syntax is going to stay in 3.4 and probably even in 3.5, I favour your proposal. We could get rid of several lines of code, those that confuse patch and my eye the most :-) Richard, would you approve such a patch at this time? I'll try to submit it by tomorrow if you like it... >>>I going to test the coss-built native compiler tonight. > > Unfortunately the cross-built compiler doesn't work :-( It seems to be > miscompiled since I get this error message: > > ./cc1 -E foo.c > <internal>:0: internal compiler error: tree check: expected class 'd', > have 'd' (function_decl) in make_decl_rtl, at varasm.c:882 > > The compiler was built by a cross-compiler built from the same source as > this one. A native 3.4 built at the beginning of September 2003 works. I > am going to test whether 3.3 and an older 3.4 as build compiler will do > better. Currently I don't know when it broke :-/ The compiler used to work fine with m68k-elf and m68k-uclinux last week (last tested from CVS sources on 20031011). -- // Bernardo Innocenti - Develer S.r.l., R&D dept. \X/ http://www.develer.com/ Please don't send Word attachments - http://www.gnu.org/philosophy/no-word-attachments.html ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Testing m68k changes on AmigaOS and Linux/m68k 2003-10-14 17:00 ` Bernardo Innocenti @ 2003-10-15 12:40 ` Gunther Nikl 2003-10-15 17:42 ` Gunther Nikl 2003-10-15 13:57 ` Gunther Nikl 1 sibling, 1 reply; 20+ messages in thread From: Gunther Nikl @ 2003-10-15 12:40 UTC (permalink / raw) To: Bernardo Innocenti; +Cc: GCC Mailing List, Richard Henderson On Tue, Oct 14, 2003 at 06:47:42PM +0200, Bernardo Innocenti wrote: > >>Have you ever tried defining MOTOROLA? If you're luckly, it should work > >>out of the box. > > > > I never tried [MOTOROLA] and I won't try it because the assembler I > > am using doesn't support it. > > I seem to recall you told me that too, but I can't remember why. > Can't you use gas? I do use gas but its an enhanced pre-bfd version which is just fine for the AmigaOS/m68k port because it's a simple a.out port. My "problem" with BFD is that read and write support is always included even if only one is needed. That makes them fat (and thus slow) on a system that doesn't have un*x style shared libraries. > Anyway, since the MIT syntax is going to stay in 3.4 and probably > even in 3.5, I favour your proposal [defining MOTOROLA to 0/1] > > We could get rid of several lines of code, those that confuse patch > and my eye the most :-) I am looking forward for your patch :-) > >>>I going to test the coss-built native compiler tonight. > > > > Unfortunately the cross-built compiler doesn't work :-( It seems to be > > miscompiled since I get this error message: > > > > ./cc1 -E foo.c > > <internal>:0: internal compiler error: tree check: expected class 'd', > > have 'd' (function_decl) in make_decl_rtl, at varasm.c:882 > > > > The compiler was built by a cross-compiler built from the same source as > > this one. A native 3.4 built at the beginning of September 2003 works. I > > am going to test whether 3.3 and an older 3.4 as build compiler will do > > better. Currently I don't know when it broke :-/ > > The compiler used to work fine with m68k-elf and m68k-uclinux last week > (last tested from CVS sources on 20031011). Are you sure? ;-) Its a bug with local variables stored in the frame without a frame-pointer which trashes the saved register set. Within GCC ggc_alloc() is affected (which then caused the failure I saw). Its easily reproducible with the AmigaOS port since there I can take the address of a local variable and pass it in a register. Without that ability the frame-pointer can't be eliminated. Anyway here is some example code which is compiled with "-O -fomit-frame-pointer -S" void __regargs bar(int *i, int *j); void foo(int i) { int j; bar(&i,&j); } 3.3 (release): _foo: subqw #8,sp movel sp,a1 lea sp@(12),a0 jbsr _bar addqw #8,sp rts 3.4 (20031013): _foo: subqw #4,sp lea sp@(-4),a1 <- should bas as with 3.3 lea sp@(8),a0 jbsr _bar addqw #4,sp rts I believe thats a bug within your patch from 2003-09-08. I only checked whether passed arguments are passed correctly. I didn't inspect local variables in the frame. I hope you can locate the problem. Gunther ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Testing m68k changes on AmigaOS and Linux/m68k 2003-10-15 12:40 ` Gunther Nikl @ 2003-10-15 17:42 ` Gunther Nikl 2003-10-15 20:53 ` Bernardo Innocenti 0 siblings, 1 reply; 20+ messages in thread From: Gunther Nikl @ 2003-10-15 17:42 UTC (permalink / raw) To: Bernardo Innocenti; +Cc: GCC Mailing List, Richard Henderson On Wed, Oct 15, 2003 at 11:32:47AM +0200, Gunther Nikl wrote: > > The compiler used to work fine with m68k-elf and m68k-uclinux last week > > (last tested from CVS sources on 20031011). > > Are you sure? ;-) Argl! Turns out that bug was a bug in my patches which still redefined ARG_POINTER_REGNUM to FRAME_POINTER_REGNUM :-/ Sorry for the false report about that issue. However, I found a bug that was hidden by the wrong test when saving registers. The wrong mask is used when saving multiple registers in m68k_output_function_prologue(). The diff includes the original change. No ChangeLog entry this time. Gunther --cut-- --- m68k.c_ Mon Oct 13 14:39:45 2003 +++ m68k.c Wed Oct 15 18:37:02 2003 @@ -966,7 +966,7 @@ m68k_output_function_prologue (FILE *str warning ("stack limit expression is not supported"); } - if (num_saved_regs <= 2) + if (current_frame.reg_no <= 2) { /* Store each separately in the same order moveml uses. Using two movel instructions instead of a single moveml @@ -1007,17 +1007,17 @@ m68k_output_function_prologue (FILE *str the fsize_with_regs amount. */ #ifdef MOTOROLA - asm_fprintf (stream, "\tmovm.l %I0x%x,(%Rsp)\n", current_frame.reg_mask); + asm_fprintf (stream, "\tmovm.l %I0x%x,(%Rsp)\n", current_frame.reg_rev_mask); #else - asm_fprintf (stream, "\tmoveml %I0x%x,%Rsp@\n", current_frame.reg_mask); + asm_fprintf (stream, "\tmoveml %I0x%x,%Rsp@\n", current_frame.reg_rev_mask); #endif } else { #ifdef MOTOROLA - asm_fprintf (stream, "\tmovm.l %I0x%x,-(%Rsp)\n", current_frame.reg_mask); + asm_fprintf (stream, "\tmovm.l %I0x%x,-(%Rsp)\n", current_frame.reg_rev_mask); #else - asm_fprintf (stream, "\tmoveml %I0x%x,%Rsp@-\n", current_frame.reg_mask); + asm_fprintf (stream, "\tmoveml %I0x%x,%Rsp@-\n", current_frame.reg_rev_mask); #endif } if (dwarf2out_do_frame ()) @@ -1029,7 +1029,7 @@ m68k_output_function_prologue (FILE *str if (! frame_pointer_needed) dwarf2out_def_cfa (l, STACK_POINTER_REGNUM, cfa_offset); for (regno = 0, n_regs = 0; regno < 16; regno++) - if (current_frame.reg_mask & (1 << regno)) + if (current_frame.reg_rev_mask & (1 << regno)) dwarf2out_reg_save (l, regno, -cfa_offset + n_regs++ * 4); } --cut-- ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Testing m68k changes on AmigaOS and Linux/m68k 2003-10-15 17:42 ` Gunther Nikl @ 2003-10-15 20:53 ` Bernardo Innocenti 2003-10-15 21:18 ` Andreas Schwab 2003-10-16 15:27 ` Gunther Nikl 0 siblings, 2 replies; 20+ messages in thread From: Bernardo Innocenti @ 2003-10-15 20:53 UTC (permalink / raw) To: Gunther Nikl; +Cc: GCC Mailing List, Richard Henderson Gunther Nikl wrote: > On Wed, Oct 15, 2003 at 11:32:47AM +0200, Gunther Nikl wrote: > >>>The compiler used to work fine with m68k-elf and m68k-uclinux last week >>>(last tested from CVS sources on 20031011). >> >> Are you sure? ;-) > > Argl! Turns out that bug was a bug in my patches which still redefined > ARG_POINTER_REGNUM to FRAME_POINTER_REGNUM :-/ Sorry for the false report > about that issue. Feew... I was sweating quite a lot trying to guess what could possibly got broken... This is what I got from your test case with -O1 -fomit-frame-pointer: foo: link.w %a6,#-4 pea -4(%a6) pea 8(%a6) jbsr bar addq.l #8,%sp unlk %a6 rts As you can see, it's using the frame pointer even though it's been disabled. The offsets are all correct, but I wonder why the FP can't be eliminated for this simple case. There could be something wrong in ELIMINABLE_REGS or CAN_ELIMINATE... > However, I found a bug that was hidden by the wrong test when saving > registers. The wrong mask is used when saving multiple registers in > m68k_output_function_prologue(). The diff includes the original change. > No ChangeLog entry this time. IIRC, the mask to be used for movem in the ColdFire is the same for both saving and restoring registers (there's no post-increment/pre-decrement in movem). In the 680x0, we need the reversed mask when storing and the straight one for restoring. > @@ -1007,17 +1007,17 @@ m68k_output_function_prologue (FILE *str > the fsize_with_regs amount. */ > > #ifdef MOTOROLA > - asm_fprintf (stream, "\tmovm.l %I0x%x,(%Rsp)\n", current_frame.reg_mask); > + asm_fprintf (stream, "\tmovm.l %I0x%x,(%Rsp)\n", current_frame.reg_rev_mask); > #else > - asm_fprintf (stream, "\tmoveml %I0x%x,%Rsp@\n", current_frame.reg_mask); > + asm_fprintf (stream, "\tmoveml %I0x%x,%Rsp@\n", current_frame.reg_rev_mask); > #endif ...so, this code was correct for the ColdFire and shouldn't be changed (I wouldn't get a working Linux kernel otherwise). > #ifdef MOTOROLA > - asm_fprintf (stream, "\tmovm.l %I0x%x,-(%Rsp)\n", current_frame.reg_mask); > + asm_fprintf (stream, "\tmovm.l %I0x%x,-(%Rsp)\n", current_frame.reg_rev_mask); > #else > - asm_fprintf (stream, "\tmoveml %I0x%x,%Rsp@-\n", current_frame.reg_mask); > + asm_fprintf (stream, "\tmoveml %I0x%x,%Rsp@-\n", current_frame.reg_rev_mask); > #endif > } > if (dwarf2out_do_frame ()) For this, please accept a mea culpa, mea culpa, mea maxima culpa :-) > @@ -1029,7 +1029,7 @@ m68k_output_function_prologue (FILE *str > if (! frame_pointer_needed) > dwarf2out_def_cfa (l, STACK_POINTER_REGNUM, cfa_offset); > for (regno = 0, n_regs = 0; regno < 16; regno++) > - if (current_frame.reg_mask & (1 << regno)) > + if (current_frame.reg_rev_mask & (1 << regno)) > dwarf2out_reg_save (l, regno, > -cfa_offset + n_regs++ * 4); > } Are you sure about this? I'm pretty sure that when regno is n, the correct bit to test with (1<<n) would be in the straight mask. What's the push order of movem on the 68000? If it pushes registers from D0 to A7, then the offset is also fine. -- // Bernardo Innocenti - Develer S.r.l., R&D dept. \X/ http://www.develer.com/ Please don't send Word attachments - http://www.gnu.org/philosophy/no-word-attachments.html ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Testing m68k changes on AmigaOS and Linux/m68k 2003-10-15 20:53 ` Bernardo Innocenti @ 2003-10-15 21:18 ` Andreas Schwab 2003-10-16 15:27 ` Gunther Nikl 1 sibling, 0 replies; 20+ messages in thread From: Andreas Schwab @ 2003-10-15 21:18 UTC (permalink / raw) To: Bernardo Innocenti; +Cc: Gunther Nikl, GCC Mailing List, Richard Henderson Bernardo Innocenti <bernie@develer.com> writes: > What's the push order of movem on the 68000? If it > pushes registers from D0 to A7, then the offset is > also fine. The order of the registers in memory is always the same, independent of the addressing mode (D0 to A7 with increasing addresses). Andreas. -- Andreas Schwab, SuSE Labs, schwab@suse.de SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Testing m68k changes on AmigaOS and Linux/m68k 2003-10-15 20:53 ` Bernardo Innocenti 2003-10-15 21:18 ` Andreas Schwab @ 2003-10-16 15:27 ` Gunther Nikl 2003-10-16 16:36 ` Andreas Schwab 2003-10-16 17:27 ` Bernardo Innocenti 1 sibling, 2 replies; 20+ messages in thread From: Gunther Nikl @ 2003-10-16 15:27 UTC (permalink / raw) To: Bernardo Innocenti; +Cc: GCC Mailing List, Richard Henderson On Wed, Oct 15, 2003 at 08:53:58PM +0200, Bernardo Innocenti wrote: > Gunther Nikl wrote: > > Argl! Turns out that bug was a bug in my patches which still redefined > > ARG_POINTER_REGNUM to FRAME_POINTER_REGNUM :-/ Sorry for the false > > report about that issue. > > Feew... I was sweating quite a lot trying to guess what could possibly > got broken... When I first inspected your patch I noticed that I had to remove the ARG_POINTER_REGNUM redefine but then I forgot doing that when your patch was committed :-/ > This is what I got from your test case with -O1 -fomit-frame-pointer: > > foo: > link.w %a6,#-4 > pea -4(%a6) > pea 8(%a6) > jbsr bar > addq.l #8,%sp > unlk %a6 > rts > > As you can see, it's using the frame pointer even though it's been > disabled. I know that behaviour. Thats why I used __regargs :) > The offsets are all correct, but I wonder why the FP can't be eliminated > for this simple case. Yes, with framepointer it was ok. I guess that the FP can't be eliminated because that would change the offset into the frame and tracking that is probably hard. > There could be something wrong in ELIMINABLE_REGS or CAN_ELIMINATE... I would like to see FP eliminated all the time when -fomit-frame-pointer is used ;-) > > However, I found a bug that was hidden by the wrong test when saving > > registers. The wrong mask is used when saving multiple registers in > > m68k_output_function_prologue(). The diff includes the original change. > > No ChangeLog entry this time. > > IIRC, the mask to be used for movem in the ColdFire is the same for both > saving and restoring registers (there's no post-increment/pre-decrement > in movem). I didn't inspect the former version in great detail thus I missed that in this part the normal mask was computed from the rev_mask for COLDFIRE. > In the 680x0, we need the reversed mask when storing and the straight one > for restoring. Good, but I fear in that case your patch is completely broken. The FPU case seems to be messed up too... > >@@ -1029,7 +1029,7 @@ m68k_output_function_prologue (FILE *str > > if (! frame_pointer_needed) > > dwarf2out_def_cfa (l, STACK_POINTER_REGNUM, cfa_offset); > > for (regno = 0, n_regs = 0; regno < 16; regno++) > >- if (current_frame.reg_mask & (1 << regno)) > >+ if (current_frame.reg_rev_mask & (1 << regno)) > > dwarf2out_reg_save (l, regno, > > -cfa_offset + n_regs++ * 4); > > } > > Are you sure about this? I'm pretty sure that when regno is n, the > correct bit to test with (1<<n) would be in the straight mask. Now I see, you changed the test compared with the old code. I looked how the mask used here was computed before and that was mask |= 1 << (15 - regno); which is (reg_rev_mask =) rmask |= 1 << (15 - regno); in your patch. Thus the new code is correct. Hm, maybe it should be as before and use reg_rev_mask for consistency within the prologue function (except COLDFIRE ;-) I changed it back to use reg_rev_mask. The same applies to the fpu prologue generation. The fpu epilogue seems to be broken as well. Please take a look at diff between 1.107 and 1.108 to see what I mean. > What's the push order of movem on the 68000? If it pushes registers from > D0 to A7, then the offset is also fine. I believe that part is fine. Gunther PS: I also moved some comments around to the new places they belong to. --cut-- --- m68k.c_ Mon Oct 13 14:39:45 2003 +++ m68k.c Thu Oct 16 14:40:41 2003 @@ -614,6 +614,9 @@ look_for_reg: return 0; } \f +/* Note that the order of the bit mask for fmovem is the opposite + of the order for movem! */ + static void m68k_compute_frame_layout (void) { @@ -682,7 +685,12 @@ m68k_initial_elimination_offset (int fro abort(); } -/* Return true if we need to save REGNO. */ +/* Refer to the array `regs_ever_live' to determine which registers + to save; `regs_ever_live[I]' is nonzero if register number I + is ever used in the function. This function is responsible for + knowing which registers should not be saved even if used. + Return true if we need to save REGNO. */ + static bool m68k_save_reg (unsigned int regno, bool interrupt_handler) { @@ -736,15 +744,7 @@ m68k_save_reg (unsigned int regno, bool /* This function generates the assembly code for function entry. STREAM is a stdio stream to output the code to. - SIZE is an int: how many units of temporary storage to allocate. - Refer to the array `regs_ever_live' to determine which registers - to save; `regs_ever_live[I]' is nonzero if register number I - is ever used in the function. This function is responsible for - knowing which registers should not be saved even if used. */ - - -/* Note that the order of the bit mask for fmovem is the opposite - of the order for movem! */ + SIZE is an int: how many units of temporary storage to allocate. */ static void m68k_output_function_prologue (FILE *stream, HOST_WIDE_INT size ATTRIBUTE_UNUSED) @@ -925,12 +925,12 @@ m68k_output_function_prologue (FILE *str if (TARGET_68881) { - if (current_frame.fpu_mask) + if (current_frame.fpu_rev_mask) { #ifdef MOTOROLA - asm_fprintf (stream, "\tfmovm %I0x%x,-(%Rsp)\n", current_frame.fpu_mask); + asm_fprintf (stream, "\tfmovm %I0x%x,-(%Rsp)\n", current_frame.fpu_rev_mask); #else - asm_fprintf (stream, "\tfmovem %I0x%x,%Rsp@-\n", current_frame.fpu_mask); + asm_fprintf (stream, "\tfmovem %I0x%x,%Rsp@-\n", current_frame.fpu_rev_mask); #endif if (dwarf2out_do_frame ()) { @@ -941,7 +941,7 @@ m68k_output_function_prologue (FILE *str if (! frame_pointer_needed) dwarf2out_def_cfa (l, STACK_POINTER_REGNUM, cfa_offset); for (regno = 16, n_regs = 0; regno < 24; regno++) - if (current_frame.fpu_mask & (1 << (regno - 16))) + if (current_frame.fpu_rev_mask & (1 << (regno - 16))) dwarf2out_reg_save (l, regno, -cfa_offset + n_regs++ * 12); } @@ -966,7 +966,7 @@ m68k_output_function_prologue (FILE *str warning ("stack limit expression is not supported"); } - if (num_saved_regs <= 2) + if (current_frame.reg_no <= 2) { /* Store each separately in the same order moveml uses. Using two movel instructions instead of a single moveml @@ -1015,9 +1015,9 @@ m68k_output_function_prologue (FILE *str else { #ifdef MOTOROLA - asm_fprintf (stream, "\tmovm.l %I0x%x,-(%Rsp)\n", current_frame.reg_mask); + asm_fprintf (stream, "\tmovm.l %I0x%x,-(%Rsp)\n", current_frame.reg_rev_mask); #else - asm_fprintf (stream, "\tmoveml %I0x%x,%Rsp@-\n", current_frame.reg_mask); + asm_fprintf (stream, "\tmoveml %I0x%x,%Rsp@-\n", current_frame.reg_rev_mask); #endif } if (dwarf2out_do_frame ()) @@ -1029,7 +1029,7 @@ m68k_output_function_prologue (FILE *str if (! frame_pointer_needed) dwarf2out_def_cfa (l, STACK_POINTER_REGNUM, cfa_offset); for (regno = 0, n_regs = 0; regno < 16; regno++) - if (current_frame.reg_mask & (1 << regno)) + if (current_frame.reg_rev_mask & (1 << (15 - regno))) dwarf2out_reg_save (l, regno, -cfa_offset + n_regs++ * 4); } @@ -1293,7 +1293,7 @@ m68k_output_function_epilogue (FILE *str } } } - if (current_frame.fpu_rev_mask) + if (current_frame.fpu_mask) { if (big) { @@ -1301,22 +1301,22 @@ m68k_output_function_epilogue (FILE *str asm_fprintf (stream, "\tfmovm -%wd(%s,%Ra1.l),%I0x%x\n", current_frame.foffset + fsize, reg_names[FRAME_POINTER_REGNUM], - current_frame.fpu_rev_mask); + current_frame.fpu_mask); #else asm_fprintf (stream, "\tfmovem %s@(-%wd,%Ra1:l),%I0x%x\n", reg_names[FRAME_POINTER_REGNUM], current_frame.foffset + fsize, - current_frame.fpu_rev_mask); + current_frame.fpu_mask); #endif } else if (restore_from_sp) { #ifdef MOTOROLA asm_fprintf (stream, "\tfmovm (%Rsp)+,%I0x%x\n", - current_frame.fpu_rev_mask); + current_frame.fpu_mask); #else asm_fprintf (stream, "\tfmovem %Rsp@+,%I0x%x\n", - current_frame.fpu_rev_mask); + current_frame.fpu_mask); #endif } else @@ -1330,7 +1330,7 @@ m68k_output_function_epilogue (FILE *str asm_fprintf (stream, "\tfmovem %s@(-%wd),%I0x%x\n", reg_names[FRAME_POINTER_REGNUM], current_frame.foffset + fsize, - current_frame.fpu_rev_mask); + current_frame.fpu_mask); #endif } } --cut-- ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Testing m68k changes on AmigaOS and Linux/m68k 2003-10-16 15:27 ` Gunther Nikl @ 2003-10-16 16:36 ` Andreas Schwab 2003-10-16 17:27 ` Bernardo Innocenti 1 sibling, 0 replies; 20+ messages in thread From: Andreas Schwab @ 2003-10-16 16:36 UTC (permalink / raw) To: Gunther Nikl; +Cc: Bernardo Innocenti, GCC Mailing List Gunther Nikl <gni@gecko.de> writes: > --- m68k.c_ Mon Oct 13 14:39:45 2003 > +++ m68k.c Thu Oct 16 14:40:41 2003 > @@ -614,6 +614,9 @@ look_for_reg: > return 0; > } > \f > +/* Note that the order of the bit mask for fmovem is the opposite > + of the order for movem! */ That's not true for the bits as they are encoded in the opcode. In both cases the CPU starts with the highest set bit, which must correspond to the first register saved/restored. That means that bit 15/7 (movem/fmovem) of the mask is D0/FP0 for register indirect and post-increment, and A7/FP7 for pre-decrement. Andreas. -- Andreas Schwab, SuSE Labs, schwab@suse.de SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Testing m68k changes on AmigaOS and Linux/m68k 2003-10-16 15:27 ` Gunther Nikl 2003-10-16 16:36 ` Andreas Schwab @ 2003-10-16 17:27 ` Bernardo Innocenti 2003-10-21 14:38 ` Gunther Nikl 1 sibling, 1 reply; 20+ messages in thread From: Bernardo Innocenti @ 2003-10-16 17:27 UTC (permalink / raw) To: Gunther Nikl; +Cc: GCC Mailing List, Richard Henderson Gunther Nikl wrote: >>Feew... I was sweating quite a lot trying to guess what could possibly >>got broken... > > When I first inspected your patch I noticed that I had to remove the > ARG_POINTER_REGNUM redefine but then I forgot doing that when your > patch was committed :-/ Aha! So I'm not the only one who has a bad memory! ;-) >>foo: >> link.w %a6,#-4 >> pea -4(%a6) >> pea 8(%a6) >> jbsr bar >> addq.l #8,%sp >> unlk %a6 >> rts >> >>As you can see, it's using the frame pointer even though it's been >>disabled. > > I know that behaviour. Thats why I used __regargs :) Hmmm... with regargs, there are no pushes on the stack making things more complicated. I wonder why the compiler is also adjusting the SP after invoking bar(). IIRC, GCC knows how to accumulate pushes from multiple function calls and should optimize then the control flow ends into the epilogue where unlink can take care of it. >>The offsets are all correct, but I wonder why the FP can't be eliminated >>for this simple case. > > Yes, with framepointer it was ok. I guess that the FP can't be eliminated > because that would change the offset into the frame and tracking that is > probably hard. The old SAS/C knew how to do that pretty well :-) It could even inline varargs functions, something that GCC still can't do. It was pretty useful for inline stubs such as DoMethod() or Printf(). >>There could be something wrong in ELIMINABLE_REGS or CAN_ELIMINATE... > > I would like to see FP eliminated all the time when -fomit-frame-pointer > is used ;-) It certainly _is_ possible, because SAS/C never used it. Perhaps it's a problem in the middle-end or perhaps we need to add more elimination pairs. The m68k back-end is currently using the same set of eliminations of the x86, therefore my guess is that it's just a missing feature. >>In the 680x0, we need the reversed mask when storing and the straight one >>for restoring. > > Good, but I fear in that case your patch is completely broken. > The FPU case seems to be messed up too... It was done like that even before my patch... is it possible that it has always been broken? >>>@@ -1029,7 +1029,7 @@ m68k_output_function_prologue (FILE *str >>> if (! frame_pointer_needed) >>> dwarf2out_def_cfa (l, STACK_POINTER_REGNUM, cfa_offset); >>> for (regno = 0, n_regs = 0; regno < 16; regno++) >>>- if (current_frame.reg_mask & (1 << regno)) >>>+ if (current_frame.reg_rev_mask & (1 << regno)) >>> dwarf2out_reg_save (l, regno, >>> -cfa_offset + n_regs++ * 4); >>> } >> >>Are you sure about this? I'm pretty sure that when regno is n, the >>correct bit to test with (1<<n) would be in the straight mask. > > Now I see, you changed the test compared with the old code. I looked how > the mask used here was computed before and that was > > mask |= 1 << (15 - regno); > > which is > > (reg_rev_mask =) rmask |= 1 << (15 - regno); > > in your patch. Thus the new code is correct. Hm, maybe it should be as > before and use reg_rev_mask for consistency within the prologue function > (except COLDFIRE ;-) I changed it back to use reg_rev_mask. I did that intentionally to make the new code more readable. I had a hard time trying to guess what the backwards loop was doing in combination with the backwards shift. > The same applies to the fpu prologue generation. The fpu epilogue seems > to be broken as well. > Please take a look at diff between 1.107 and 1.108 to see what I mean. Yes, you're right. The loop in m68k_compute_frame_layout() was reversed for some reason (Peter Barada wrote it): for (regno = 16; regno < 24; regno++) if (m68k_save_reg (regno, interrupt_handler)) { mask |= 1 << (23 - regno); rmask |= 1 << (regno - 16); saved++; } [...] current_frame.fpu_mask = mask; current_frame.fpu_rev_mask = rmask; So, fpu_mask is actually the _reversed_ mask. The old code in m68k_output_function_prologue() computed the FP mask like this: for (regno = 16; regno < 24; regno++) if (m68k_save_reg (regno, interrupt_handler)) { mask |= 1 << (regno - 16); num_saved_regs++; } This is _not_ a reversed mask! Old epilogue used to compute a revered mask for the FP regs: for (regno = 16; regno < 24; regno++) if (m68k_save_reg (regno, interrupt_handler)) { nregs++; fmask |= 1 << (23 - regno); } So I agree with you. Both the epilogue and the prologue are doing the opposite of what they should do. Your patch looks fine, but for clarity I'd rather fix the problem by reversing the loop in m68k_compute_frame_layout(). > PS: I also moved some comments around to the new places they belong to. That's great, thank you! > @@ -682,7 +685,12 @@ m68k_initial_elimination_offset (int fro > abort(); > } > > -/* Return true if we need to save REGNO. */ > +/* Refer to the array `regs_ever_live' to determine which registers > + to save; `regs_ever_live[I]' is nonzero if register number I > + is ever used in the function. This function is responsible for > + knowing which registers should not be saved even if used. > + Return true if we need to save REGNO. */ ^^^ The coding standard requires two spaces after the '.'. Could you please post the revised patch to gcc-patches for approval? I will commit it for you ASAP. GCC's front page still says that 3.4 is in stage 2. If we're lucky we can still get this in without opening a PR :-) -- // Bernardo Innocenti - Develer S.r.l., R&D dept. \X/ http://www.develer.com/ Please don't send Word attachments - http://www.gnu.org/philosophy/no-word-attachments.html ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Testing m68k changes on AmigaOS and Linux/m68k 2003-10-16 17:27 ` Bernardo Innocenti @ 2003-10-21 14:38 ` Gunther Nikl 2003-10-21 20:33 ` Bernardo Innocenti 0 siblings, 1 reply; 20+ messages in thread From: Gunther Nikl @ 2003-10-21 14:38 UTC (permalink / raw) To: Bernardo Innocenti; +Cc: GCC Mailing List, Richard Henderson On Thu, Oct 16, 2003 at 07:00:14PM +0200, Bernardo Innocenti wrote: > Hmmm... with regargs, there are no pushes on the stack making things more > complicated. Why does that make thinks more complicate? I would say its the opposite. > IIRC, GCC knows how to accumulate pushes from multiple function calls and > should optimize then the control flow ends into the epilogue where unlink > can take care of it. This is probably a missed optimization case. > > Yes, with framepointer it was ok. I guess that the FP can't be eliminated > > because that would change the offset into the frame and tracking that is > > probably hard. > > The old SAS/C knew how to do that pretty well :-) SAS/C started to use SP-only with version 6 and they needed several subreleases to squeeze the last bugs out for it to work reliable according to their releasenotes. > It could even inline varargs functions, something that GCC still can't do. > It was pretty useful for inline stubs such as DoMethod() or Printf(). Printf() is handled by a pragma, but indeed DoMethod() can be inlined. > Could you please post the revised patch to gcc-patches for approval? > GCC's front page still says that 3.4 is in stage 2. If we're lucky we > can still get this in without opening a PR :-) 3.4 is in stage 3. Does that mean that a PR is required now? The attached patch fixes the regressions caused by the switch to always use the computed values from m68k_compute_frame_layout. Gunther --cut-- 2003-10-21 Gunther Nikl <gni@gecko.de> * config/m68k/m68k.c (m68k_compute_frame_layout): swap reg_mask and reg_rev_mask computation * config/m68k/m68k.c (m68k_output_function_prologue): Fix usage of current_frame (one typo and one missing); use reg_rev_mask not reg_mask * config/m68k/m68k.c (m68k_output_function_epilogue): Fix usage of current_frame; use fpu_rev_mask not fpu_mask --- m68k.c.orig Tue Oct 21 13:31:13 2003 +++ m68k.c Tue Oct 21 13:32:35 2003 @@ -358,8 +358,8 @@ m68k_compute_frame_layout (void) for (regno = 16; regno < 24; regno++) if (m68k_save_reg (regno, interrupt_handler)) { - mask |= 1 << (23 - regno); - rmask |= 1 << (regno - 16); + mask |= 1 << (regno - 16); + rmask |= 1 << (23 - regno); saved++; } current_frame.foffset = saved * 12 /* (TARGET_CFV4E ? 8 : 12) */; @@ -616,7 +616,7 @@ m68k_output_function_prologue (FILE *str #ifdef MOTOROLA asm_fprintf (stream, "\tfmovm %I0x%x,-(%Rsp)\n", current_frame.fpu_mask); #else - asm_fprintf (stream, "\tfmovem %I0x%x,%Rsp@-\n", current_frmae.fpu_mask); + asm_fprintf (stream, "\tfmovem %I0x%x,%Rsp@-\n", current_frame.fpu_mask); #endif if (dwarf2out_do_frame ()) { @@ -651,8 +651,8 @@ m68k_output_function_prologue (FILE *str else if (GET_CODE (stack_limit_rtx) != SYMBOL_REF) warning ("stack limit expression is not supported"); } - - if (num_saved_regs <= 2) + + if (current_frame.reg_no <= 2) { /* Store each separately in the same order moveml uses. Using two movel instructions instead of a single moveml @@ -701,9 +701,9 @@ m68k_output_function_prologue (FILE *str else { #ifdef MOTOROLA - asm_fprintf (stream, "\tmovm.l %I0x%x,-(%Rsp)\n", current_frame.reg_mask); + asm_fprintf (stream, "\tmovm.l %I0x%x,-(%Rsp)\n", current_frame.reg_rev_mask); #else - asm_fprintf (stream, "\tmoveml %I0x%x,%Rsp@-\n", current_frame.reg_mask); + asm_fprintf (stream, "\tmoveml %I0x%x,%Rsp@-\n", current_frame.reg_rev_mask); #endif } if (dwarf2out_do_frame ()) @@ -928,7 +928,7 @@ m68k_output_function_epilogue (FILE *str #else asm_fprintf (stream, "\tmoveml %s@(-%wd),%I0x%x\n", reg_names[FRAME_POINTER_REGNUM], - offset + fsize, + current_frame.offset + fsize, current_frame.reg_mask); #endif } @@ -1007,7 +1007,7 @@ m68k_output_function_epilogue (FILE *str asm_fprintf (stream, "\tfmovm -%wd(%s),%I0x%x\n", current_frame.foffset + fsize, reg_names[FRAME_POINTER_REGNUM], - current_frame.fpu_mask); + current_frame.fpu_rev_mask); #else asm_fprintf (stream, "\tfmovem %s@(-%wd),%I0x%x\n", reg_names[FRAME_POINTER_REGNUM], --cut-- ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Testing m68k changes on AmigaOS and Linux/m68k 2003-10-21 14:38 ` Gunther Nikl @ 2003-10-21 20:33 ` Bernardo Innocenti 2003-10-23 16:11 ` Gunther Nikl 0 siblings, 1 reply; 20+ messages in thread From: Bernardo Innocenti @ 2003-10-21 20:33 UTC (permalink / raw) To: Gunther Nikl; +Cc: GCC Mailing List, Richard Henderson Gunther Nikl wrote: > On Thu, Oct 16, 2003 at 07:00:14PM +0200, Bernardo Innocenti wrote: > >>Hmmm... with regargs, there are no pushes on the stack making things more >>complicated. > > Why does that make thinks more complicate? I would say its the opposite. I do agree with you, I've just omitted the punctuation. "making things more complicated" refers to "pushes on the stack", not to "regargs" ;-) >>> Yes, with framepointer it was ok. I guess that the FP can't be eliminated >>> because that would change the offset into the frame and tracking that is >>> probably hard. >> >>The old SAS/C knew how to do that pretty well :-) > > SAS/C started to use SP-only with version 6 and they needed several > subreleases to squeeze the last bugs out for it to work reliable > according to their releasenotes. Yes, but most compilers weren't that much reliable at the time... code generation bugs were quite ordinary ;-) >>It could even inline varargs functions, something that GCC still can't do. >>It was pretty useful for inline stubs such as DoMethod() or Printf(). > > Printf() is handled by a pragma, but indeed DoMethod() can be inlined. Several years ago I wrote this header file: http://www.codewiz.org/projects/amiga/Headers/BoopsiStubs.h I used it in several projects using BOOPSI classes. As you can see, I had two sets of macros because GCC did not want to inline the varargs functions. I wonder if GCC 3.4 would do it... >>Could you please post the revised patch to gcc-patches for approval? >>GCC's front page still says that 3.4 is in stage 2. If we're lucky we >>can still get this in without opening a PR :-) > > 3.4 is in stage 3. Does that mean that a PR is required now? I think it's not needed, as long as the patch is not introducing new functionality. Bugfixes, documentation improvements and cleanups are still acceptable in stage 3. > 2003-10-21 Gunther Nikl <gni@gecko.de> > > * config/m68k/m68k.c (m68k_compute_frame_layout): swap reg_mask and > reg_rev_mask computation > * config/m68k/m68k.c (m68k_output_function_prologue): Fix usage of > current_frame (one typo and one missing); use reg_rev_mask not > reg_mask > * config/m68k/m68k.c (m68k_output_function_epilogue): Fix usage of > current_frame; use fpu_rev_mask not fpu_mask A small note: ChangeLog comments should be capitalized and should end with a '.'. Don't worry, I'll fix it when I commit the patch... Everything else looks fine to me. -- // Bernardo Innocenti - Develer S.r.l., R&D dept. \X/ http://www.develer.com/ Please don't send Word attachments - http://www.gnu.org/philosophy/no-word-attachments.html ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Testing m68k changes on AmigaOS and Linux/m68k 2003-10-21 20:33 ` Bernardo Innocenti @ 2003-10-23 16:11 ` Gunther Nikl 2003-10-23 20:30 ` Bernardo Innocenti 0 siblings, 1 reply; 20+ messages in thread From: Gunther Nikl @ 2003-10-23 16:11 UTC (permalink / raw) To: Bernardo Innocenti; +Cc: GCC Mailing List, Richard Henderson On Tue, Oct 21, 2003 at 07:46:05PM +0200, Bernardo Innocenti wrote: > Several years ago I wrote this header file: > > http://www.codewiz.org/projects/amiga/Headers/BoopsiStubs.h > > I used it in several projects using BOOPSI classes. As you can see, > I had two sets of macros because GCC did not want to inline the > varargs functions. The approach with the array is nasty :-/ Every usage of such macro will create a distinc array and this will eat up stackspace. Thats why I always disabled these macros. > A small note: ChangeLog comments should be capitalized and should end > with a '.'. Don't worry, I'll fix it when I commit the patch... I will (hopefully) respect this rule the next time I submit a patch. > Everything else looks fine to me. Thank you! :-) Gunther ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Testing m68k changes on AmigaOS and Linux/m68k 2003-10-23 16:11 ` Gunther Nikl @ 2003-10-23 20:30 ` Bernardo Innocenti 2003-10-24 13:49 ` Gunther Nikl 0 siblings, 1 reply; 20+ messages in thread From: Bernardo Innocenti @ 2003-10-23 20:30 UTC (permalink / raw) To: Gunther Nikl; +Cc: GCC Mailing List, Richard Henderson Gunther Nikl wrote: >>I used it in several projects using BOOPSI classes. As you can see, >>I had two sets of macros because GCC did not want to inline the >>varargs functions. > > The approach with the array is nasty :-/ Every usage of such macro > will create a distinc array and this will eat up stackspace. Thats > why I always disabled these macros. I don't quite understand. There's (almost) no difference in stack space usage between this: { int v[] = { 1, 2, 3, 4, 5 }; extern foo(int *); foo(v); } ...and this: { extern foo(...); foo(1, 2, 3, 4, 5); } With the varargs form, the compiler will push all arguments on the stack before calling foo(), effectively wasting the same amount of stack space of the local array. When I had to pass a taglist made entirely of constant values, I used to do this instead: { static const int tags[] = { XA_Foo, 1, XA_Bar, 2, XA_Baz, 3, TAG_DONE } SetAttrsA(o, tags); } -- // Bernardo Innocenti - Develer S.r.l., R&D dept. \X/ http://www.develer.com/ Please don't send Word attachments - http://www.gnu.org/philosophy/no-word-attachments.html ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Testing m68k changes on AmigaOS and Linux/m68k 2003-10-23 20:30 ` Bernardo Innocenti @ 2003-10-24 13:49 ` Gunther Nikl 2003-10-25 6:04 ` Bernardo Innocenti 0 siblings, 1 reply; 20+ messages in thread From: Gunther Nikl @ 2003-10-24 13:49 UTC (permalink / raw) To: Bernardo Innocenti; +Cc: GCC Mailing List On Thu, Oct 23, 2003 at 07:42:24PM +0200, Bernardo Innocenti wrote: > Gunther Nikl wrote: > > The approach with the array is nasty :-/ Every usage of such macro > > will create a distinc array and this will eat up stackspace. Thats > > why I always disabled these macros. > > I don't quite understand. There's (almost) no difference in stack ^^^^^^ Thats the import difference. > space usage between this: > > { > int v[] = { 1, 2, 3, 4, 5 }; > extern foo(int *); > foo(v); > } > > ...and this: > > { > extern foo(...); > foo(1, 2, 3, 4, 5); > } > > With the varargs form, the compiler will push all arguments on the > stack before calling foo(), effectively wasting the same amount of > stack space of the local array. The array is allocated at _frame generation_. Now imagine using a lot of such calls where every array is only used _one_ time. The varargs form uses the stackspace only temorary when making the call and cleans up soon after the call returns. > When I had to pass a taglist made entirely of constant values, I used to > do this instead: > > { > static const int tags[] = {...}; > ... > } This is of course much better since the array is created a compile time and placed in readonly-memory. Gunther ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Testing m68k changes on AmigaOS and Linux/m68k 2003-10-24 13:49 ` Gunther Nikl @ 2003-10-25 6:04 ` Bernardo Innocenti 0 siblings, 0 replies; 20+ messages in thread From: Bernardo Innocenti @ 2003-10-25 6:04 UTC (permalink / raw) To: Gunther Nikl; +Cc: GCC Mailing List Gunther Nikl wrote: >>With the varargs form, the compiler will push all arguments on the >>stack before calling foo(), effectively wasting the same amount of >>stack space of the local array. > > The array is allocated at _frame generation_. Now imagine using a lot > of such calls where every array is only used _one_ time. The varargs > form uses the stackspace only temorary when making the call and cleans > up soon after the call returns. I should have guessed GCC would handle it like that. However, since the scope of the array is within a nested block, a smarter compiler might do the right thing. That's another missing optimization. And, btw, SAS/C did it right ;-) -- // Bernardo Innocenti - Develer S.r.l., R&D dept. \X/ http://www.develer.com/ Please don't send Word attachments - http://www.gnu.org/philosophy/no-word-attachments.html ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Testing m68k changes on AmigaOS and Linux/m68k 2003-10-14 17:00 ` Bernardo Innocenti 2003-10-15 12:40 ` Gunther Nikl @ 2003-10-15 13:57 ` Gunther Nikl 1 sibling, 0 replies; 20+ messages in thread From: Gunther Nikl @ 2003-10-15 13:57 UTC (permalink / raw) To: Bernardo Innocenti; +Cc: GCC Mailing List, Richard Henderson On Tue, Oct 14, 2003 at 06:47:42PM +0200, Bernardo Innocenti wrote: > I've recently applied all my remaining m68k patches. There is another buglet in your patches. Gunther --cut-- 2003-10-15 Gunther Nikl <gni@gecko.de> * config/m68k/m68k.c (m68k_output_function_prologue): use value from current_frame when saving registers --- m68k.c~ Mon Oct 13 14:39:45 2003 +++ m68k.c Wed Oct 15 11:59:27 2003 @@ -966,7 +966,7 @@ m68k_output_function_prologue (FILE *str warning ("stack limit expression is not supported"); } - if (num_saved_regs <= 2) + if (current_frame.reg_no <= 2) { /* Store each separately in the same order moveml uses. Using two movel instructions instead of a single moveml --cut-- ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: Testing m68k changes on AmigaOS and Linux/m68k 2003-10-12 4:07 Testing m68k changes on AmigaOS and Linux/m68k Bernardo Innocenti 2003-10-13 17:24 ` Gunther Nikl @ 2003-10-31 23:47 ` Matthias Klose 1 sibling, 0 replies; 20+ messages in thread From: Matthias Klose @ 2003-10-31 23:47 UTC (permalink / raw) To: Bernardo Innocenti; +Cc: Gunther Nikl, GCC Mailing List Bernardo Innocenti writes: > Hello Gunther, > > I've recently applied all my remaining m68k patches. > > Since I can only test on ColdFire targets, would you > mind doing a bootstrap on Linux/m68k and/or AmigaOS? Please see PR12371 for bootstrap comparision failures on m68k-linux, probably introduced between 20030901 and 20030904. ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2003-10-31 19:42 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2003-10-12 4:07 Testing m68k changes on AmigaOS and Linux/m68k Bernardo Innocenti 2003-10-13 17:24 ` Gunther Nikl 2003-10-14 12:40 ` Bernardo Innocenti 2003-10-14 13:56 ` Gunther Nikl 2003-10-14 17:00 ` Bernardo Innocenti 2003-10-15 12:40 ` Gunther Nikl 2003-10-15 17:42 ` Gunther Nikl 2003-10-15 20:53 ` Bernardo Innocenti 2003-10-15 21:18 ` Andreas Schwab 2003-10-16 15:27 ` Gunther Nikl 2003-10-16 16:36 ` Andreas Schwab 2003-10-16 17:27 ` Bernardo Innocenti 2003-10-21 14:38 ` Gunther Nikl 2003-10-21 20:33 ` Bernardo Innocenti 2003-10-23 16:11 ` Gunther Nikl 2003-10-23 20:30 ` Bernardo Innocenti 2003-10-24 13:49 ` Gunther Nikl 2003-10-25 6:04 ` Bernardo Innocenti 2003-10-15 13:57 ` Gunther Nikl 2003-10-31 23:47 ` Matthias Klose
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).