This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Testing m68k changes on AmigaOS and Linux/m68k

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! ;-)

      link.w %a6,#-4
      pea -4(%a6)
      pea 8(%a6)
      jbsr bar
      addq.l #8,%sp
      unlk %a6

As you can see, it's using the frame pointer even though it's been

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

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))
          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
-/* 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.

Please don't send Word attachments -

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]