Re: Testing m68k changes on AmigaOS and Linux/m68k

[Bernardo Innocenti <bernie@develer.com>]

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