Testing m68k changes on AmigaOS and Linux/m68k

Testing m68k changes on AmigaOS and Linux/m68k

[Bernardo Innocenti]

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

Re: Testing m68k changes on AmigaOS and Linux/m68k

[Gunther Nikl]

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--

Re: Testing m68k changes on AmigaOS and Linux/m68k

[Bernardo Innocenti]

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

Re: Testing m68k changes on AmigaOS and Linux/m68k

[Gunther Nikl]

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

Re: Testing m68k changes on AmigaOS and Linux/m68k

[Bernardo Innocenti]

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

Re: Testing m68k changes on AmigaOS and Linux/m68k

[Gunther Nikl]

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

Re: Testing m68k changes on AmigaOS and Linux/m68k

[Gunther Nikl]

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--

Re: Testing m68k changes on AmigaOS and Linux/m68k

[Gunther Nikl]

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--

Re: Testing m68k changes on AmigaOS and Linux/m68k

[Bernardo Innocenti]

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

Re: Testing m68k changes on AmigaOS and Linux/m68k

[Andreas Schwab]

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."

Re: Testing m68k changes on AmigaOS and Linux/m68k

[Gunther Nikl]

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--

Re: Testing m68k changes on AmigaOS and Linux/m68k

[Andreas Schwab]

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."

Re: Testing m68k changes on AmigaOS and Linux/m68k

[Bernardo Innocenti]

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

Re: Testing m68k changes on AmigaOS and Linux/m68k

[Gunther Nikl]

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--

Re: Testing m68k changes on AmigaOS and Linux/m68k

[Bernardo Innocenti]

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

Re: Testing m68k changes on AmigaOS and Linux/m68k

[Gunther Nikl]

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

Re: Testing m68k changes on AmigaOS and Linux/m68k

[Bernardo Innocenti]

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

Re: Testing m68k changes on AmigaOS and Linux/m68k

[Gunther Nikl]

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

Re: Testing m68k changes on AmigaOS and Linux/m68k

[Bernardo Innocenti]

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

Re: Testing m68k changes on AmigaOS and Linux/m68k

[Matthias Klose]

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.