RFA: Add an FPIC set of multilibs to the MN10300 port

RFA: Add an FPIC set of multilibs to the MN10300 port

[Nick Clifton]

Hi Jeff, Hi Alex,

  I believe that there is a need for a new set of multilibs for the
  MN10300 port.  The -fpic and -fPIC command line options change the
  ABI by making register r6 fixed instead of call_saved.  This
  resulted in the problem with __main() in crt1.c that I reported
  earlier and I now have a test case which demonstrates the problem in
  normal C code:
  
    #include <stdlib.h>
    #include <stdio.h>

    #define array_size 128

    int array[array_size];
    int ascend = 1;

    static int 
    comp (const void * _a, const void * _b)
    {
      int * a = (int *) _a;
      int * b = (int *) _b;

      return ascend ? *a - *b : *b - *a;
    }

    int 
    main (void)
    {
      int i;

      for (i = array_size; i--;)
        array[i] = i;

      qsort (array, array_size, sizeof * array, comp);

      for (i = array_size; i--;)
        if (array[i] != (ascend ? i : (array_size - i) - 1))
          abort ();

      exit (0);
    }

  Compile this program without -fPIC and run it and it will exit
  successfully, compile it with -fPIC and it will seg fault because
  the comp() function will corrupt the value held in register a2 which
  the qsort() library function expects will be preserved.

  I have attached a patch to add a set of pic multilibs.  Please may I
  apply it ?

Cheers
  Nick

gcc/ChangeLog
2008-10-14  Nick Clifton  <nickc@redhat.com>

	* config/mn10300/t-mn10300 (MULTILIB_OPTIONS): Add fPIC.
	(MULTILIB_DIRNAMES): Add pic.
	(MULTILIB_MATCHES): New.  Treat -fpic as -fPIC.

Index: gcc/config/mn10300/t-mn10300
===================================================================
--- gcc/config/mn10300/t-mn10300	(revision 141104)
+++ gcc/config/mn10300/t-mn10300	(working copy)
@@ -10,8 +10,9 @@
 	echo '#define FLOAT' > fp-bit.c
 	cat $(srcdir)/config/fp-bit.c >> fp-bit.c
 
-MULTILIB_OPTIONS = mam33/mam33-2
-MULTILIB_DIRNAMES = am33 am33-2
+MULTILIB_OPTIONS = mam33/mam33-2 fPIC
+MULTILIB_DIRNAMES = am33 am33-2  pic
+MULTILIB_MATCHES  = fPIC=fpic
 
 LIBGCC = stmp-multilib
 INSTALL_LIBGCC = install-multilib

Re: RFA: Add an FPIC set of multilibs to the MN10300 port

[Jeff Law]

Nick Clifton wrote:
> Hi Jeff, Hi Alex,
>
>   I believe that there is a need for a new set of multilibs for the
>   MN10300 port.  The -fpic and -fPIC command line options change the
>   ABI by making register r6 fixed instead of call_saved.  This
>   resulted in the problem with __main() in crt1.c that I reported
>   earlier and I now have a test case which demonstrates the problem in
>   normal C code:
>   
>     #include <stdlib.h>
>     #include <stdio.h>
>
>     #define array_size 128
>
>     int array[array_size];
>     int ascend = 1;
>
>     static int 
>     comp (const void * _a, const void * _b)
>     {
>       int * a = (int *) _a;
>       int * b = (int *) _b;
>
>       return ascend ? *a - *b : *b - *a;
>     }
>
>     int 
>     main (void)
>     {
>       int i;
>
>       for (i = array_size; i--;)
>         array[i] = i;
>
>       qsort (array, array_size, sizeof * array, comp);
>
>       for (i = array_size; i--;)
>         if (array[i] != (ascend ? i : (array_size - i) - 1))
>           abort ();
>
>       exit (0);
>     }
>
>   Compile this program without -fPIC and run it and it will exit
>   successfully, compile it with -fPIC and it will seg fault because
>   the comp() function will corrupt the value held in register a2 which
>   the qsort() library function expects will be preserved.
>
>   I have attached a patch to add a set of pic multilibs.  Please may I
>   apply it ?
>
> Cheers
>   Nick
>
> gcc/ChangeLog
> 2008-10-14  Nick Clifton  <nickc@redhat.com>
>
> 	* config/mn10300/t-mn10300 (MULTILIB_OPTIONS): Add fPIC.
> 	(MULTILIB_DIRNAMES): Add pic.
> 	(MULTILIB_MATCHES): New.  Treat -fpic as -fPIC.
>   
Something doesn't make sense here.   a2 is call-saved and thus a 
function can reasonably expect its value to be preserved across calls -- 
which ought to apply to PIC code as well.

So ISTM that if comp is modifying a2 in a way which is visible to qsort, 
then the problem would be that a2 isn't being saved/restored within comp.

At least that's the way it seems to me and how things work on other 
ports.  Alex -- is there something special in the mn103 ABI for PIC code 
that I'm not aware of?

Jeff

Re: RFA: Add an FPIC set of multilibs to the MN10300 port

[Nick Clifton]

[-- Attachment #1: Type: text/plain, Size: 2316 bytes --]

Hi Jeff,

 > Something doesn't make sense here.   a2 is call-saved and thus a
 > function can reasonably expect its value to be preserved across calls
 > -- which ought to apply to PIC code as well.
 >
 > So ISTM that if comp is modifying a2 in a way which is visible to
 > qsort, then the problem would be that a2 isn't being saved/restored
 > within comp.
 >
 > At least that's the way it seems to me and how things work on other
 > ports.

A fair point.  In which case please may I offer an alternative solution?

The problem it seems to me is that when a register is marked as fixed it 
also has to be marked as call-used.  So for the mn103000 marking the a2 
register as fixed in pic mode is wrong because it changes it from 
call-saved to call-used.

The reason for marking a2 as fixed is so that it can be used to hold the 
GOT offset for the current function and to prevent gcc from using it for 
any other purpose.  But I think that there is another way to stop gcc 
from using a2 - remove it from the reg_alloc_order[] array.  This means 
that the register is still call-saved, and it will be correctly pushed 
and popped in any function that uses it for a GOT offset, but its 
absence from reg_alloc_order means that gcc will not try to use it in 
normal code generation.  (If a pic compiled function does not need to 
create a GOT pointer then a2 will not be used and it will not be pushed 
or popped in the function's prologue/epilogue, which is a waste of a 
register, but it is no worse than the current situation where a2 is 
marked as fixed).

So the attached patch implements this idea.  I tried to run a gcc 
regression test of an mn10300-elf toolchain but the pre-patch version 
takes forever to run.  (I gave up after 24 hours.  The tests were still 
in the gcc.c-torture/execute/execute.exp section).  In contrast the 
post-patch version runs to completion in an hour!

So, is this version of the patch OK to apply ?

Cheers
   Nick

gcc/ChangeLog
2008-10-23  Nick Clifton  <nickc@redhat.com>

     * config/mn10300/mn10300.h (CONDITIONAL_REGISTER_USAGE): In pic
     mode remove the pic register from the allocation list so that it
     cannot be used by gcc.  Do not make it fixed as this stops it from
     being call-saved, which is needed for compatibility with non-pic
     object files.

[-- Attachment #2: mn10300.h.patch --]
[-- Type: text/plain, Size: 1041 bytes --]

Index: gcc/config/mn10300/mn10300.h
===================================================================
--- gcc/config/mn10300/mn10300.h	(revision 141324)
+++ gcc/config/mn10300/mn10300.h	(working copy)
@@ -171,6 +171,8 @@
   , 0, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1 \
   }
 
+/* NB: If this array is changed check and if necessary update
+   the flag_pic case in CONDITIONAL_REGISTER_USAGE below.  */
 #define REG_ALLOC_ORDER \
   { 0, 1, 4, 5, 2, 3, 6, 7, 10, 11, 12, 13, 14, 15, 16, 17, 8, 9 \
   , 42, 43, 44, 45, 46, 47, 48, 49, 34, 35, 36, 37, 38, 39, 40, 41 \
@@ -195,8 +197,10 @@
 	fixed_regs[i] = call_used_regs[i] = 1;	\
     }						\
   if (flag_pic)					\
-    fixed_regs[PIC_OFFSET_TABLE_REGNUM] =       \
-    call_used_regs[PIC_OFFSET_TABLE_REGNUM] = 1;\
+    /* Remove PIC_REG from register allocation  \
+       list so that gcc will not use it.  Do not\
+       make it fixed as this changes the ABI.*/ \
+    reg_alloc_order[3] = 4;			\
 }
 
 /* Return number of consecutive hard regs needed starting at reg REGNO

Re: RFA: Add an FPIC set of multilibs to the MN10300 port

[Richard Sandiford]

Nick Clifton <nickc@redhat.com> writes:
> Hi Jeff,
>
>  > Something doesn't make sense here.   a2 is call-saved and thus a
>  > function can reasonably expect its value to be preserved across calls
>  > -- which ought to apply to PIC code as well.
>  >
>  > So ISTM that if comp is modifying a2 in a way which is visible to
>  > qsort, then the problem would be that a2 isn't being saved/restored
>  > within comp.
>  >
>  > At least that's the way it seems to me and how things work on other
>  > ports.
>
> A fair point.  In which case please may I offer an alternative solution?
>
> The problem it seems to me is that when a register is marked as fixed it 
> also has to be marked as call-used.  So for the mn103000 marking the a2 
> register as fixed in pic mode is wrong because it changes it from 
> call-saved to call-used.

Have you tried using CALL_REALLY_USED_REGISTERS?  I.e. define
CALL_REALLY_USED_REGISTERS to be the same as CALL_USED_REGISTERS,
except that the former marks a2 as call-saved.

Richard

Re: RFA: Add an FPIC set of multilibs to the MN10300 port

[Nick Clifton]

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

Hi Richard,

> Have you tried using CALL_REALLY_USED_REGISTERS?  I.e. define
> CALL_REALLY_USED_REGISTERS to be the same as CALL_USED_REGISTERS,
> except that the former marks a2 as call-saved.

I had not considered this (because I did not know of the existence of 
the macro), but you are right, it is a good idea.

So Jeff, Alex - here is a third version of the patch, using Richard's 
suggestion and updating the code in mn10300.c to check the 
call_really_used[] array rather than the call_used[] array.  Tested with 
an mn10300-elf toolchain with no regressions in the gcc testsuite and a 
lot of improvements for the results when compiling with -fpic.

May I apply this version of the patch please ?

Cheers
   Nick

gcc/ChangeLog
2008-10-27  Nick Clifton  <nickc@redhat.com>

	* config/mn10300/mn10300.h (CALL_REALLY_USED_REGISTERS): Define.
	* config/mn10300/mn10300.c (fp_regs_to_save): Test the
	call_really_used_regs array rather than the call_used_regs array.
	(mn10300_get_live_callee_saved_regs, expand_prologue,
	expand_epilogue, output_tst): Likewise.


[-- Attachment #2: mn10300.pic.patch --]
[-- Type: text/plain, Size: 3735 bytes --]

Index: gcc/config/mn10300/mn10300.h
===================================================================
--- gcc/config/mn10300/mn10300.h	(revision 141372)
+++ gcc/config/mn10300/mn10300.h	(working copy)
@@ -171,6 +171,13 @@
   , 0, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1 \
   }
 
+/* Note: The definition of CALL_REALLY_USED_REGISTERS is not
+   redundant.  It is needed when compiling in PIC mode because
+   the a2 register becomes fixed (and hence must be marked as
+   call_used) but in order to preserve the ABI it is not marked
+   as call_really_used.  */
+#define CALL_REALLY_USED_REGISTERS CALL_USED_REGISTERS
+
 #define REG_ALLOC_ORDER \
   { 0, 1, 4, 5, 2, 3, 6, 7, 10, 11, 12, 13, 14, 15, 16, 17, 8, 9 \
   , 42, 43, 44, 45, 46, 47, 48, 49, 34, 35, 36, 37, 38, 39, 40, 41 \
Index: gcc/config/mn10300/mn10300.c
===================================================================
--- gcc/config/mn10300/mn10300.c	(revision 141324)
+++ gcc/config/mn10300/mn10300.c	(working copy)
@@ -540,7 +540,7 @@
     return 0;
 
   for (i = FIRST_FP_REGNUM; i <= LAST_FP_REGNUM; ++i)
-    if (df_regs_ever_live_p (i) && ! call_used_regs[i])
+    if (df_regs_ever_live_p (i) && ! call_really_used_regs[i])
       ++n;
 
   return n;
@@ -617,7 +617,7 @@
 
   mask = 0;
   for (i = 0; i <= LAST_EXTENDED_REGNUM; i++)
-    if (df_regs_ever_live_p (i) && ! call_used_regs[i])
+    if (df_regs_ever_live_p (i) && ! call_really_used_regs[i])
       mask |= (1 << i);
   if ((mask & 0x3c000) != 0)
     mask |= 0x3c000;
@@ -804,7 +804,7 @@
 	 frame pointer, size is nonzero and the user hasn't
 	 changed the calling conventions of a0.  */
       if (! frame_pointer_needed && size
-	  && call_used_regs[FIRST_ADDRESS_REGNUM]
+	  && call_really_used_regs [FIRST_ADDRESS_REGNUM]
 	  && ! fixed_regs[FIRST_ADDRESS_REGNUM])
 	{
 	  /* Insn: add -(size + 4 * num_regs_to_save), sp.  */
@@ -828,7 +828,7 @@
 
       /* Consider alternative save_a0_no_merge if the user hasn't
 	 changed the calling conventions of a0.  */
-      if (call_used_regs[FIRST_ADDRESS_REGNUM]
+      if (call_really_used_regs [FIRST_ADDRESS_REGNUM]
 	  && ! fixed_regs[FIRST_ADDRESS_REGNUM])
 	{
 	  /* Insn: add -4 * num_regs_to_save, sp.  */
@@ -910,7 +910,7 @@
 
       /* Now actually save the FP registers.  */
       for (i = FIRST_FP_REGNUM; i <= LAST_FP_REGNUM; ++i)
-	if (df_regs_ever_live_p (i) && ! call_used_regs[i])
+	if (df_regs_ever_live_p (i) && ! call_really_used_regs [i])
 	  {
 	    rtx addr;
 
@@ -1046,7 +1046,7 @@
 
 	  /* Consider using a1 in post-increment mode, as long as the
 	     user hasn't changed the calling conventions of a1.  */
-	  if (call_used_regs[FIRST_ADDRESS_REGNUM+1]
+	  if (call_really_used_regs [FIRST_ADDRESS_REGNUM + 1]
 	      && ! fixed_regs[FIRST_ADDRESS_REGNUM+1])
 	    {
 	      /* Insn: mov sp,a1.  */
@@ -1114,7 +1114,7 @@
 	reg = gen_rtx_POST_INC (SImode, reg);
 
       for (i = FIRST_FP_REGNUM; i <= LAST_FP_REGNUM; ++i)
-	if (df_regs_ever_live_p (i) && ! call_used_regs[i])
+	if (df_regs_ever_live_p (i) && ! call_really_used_regs [i])
 	  {
 	    rtx addr;
 
@@ -1687,7 +1687,7 @@
 	  && REGNO_REG_CLASS (REGNO (SET_DEST (set))) != EXTENDED_REGS
 	  && REGNO (SET_DEST (set)) != REGNO (operand)
 	  && (!past_call
-	      || !call_used_regs[REGNO (SET_DEST (set))]))
+	      || ! call_really_used_regs [REGNO (SET_DEST (set))]))
 	{
 	  rtx xoperands[2];
 	  xoperands[0] = operand;
@@ -1706,7 +1706,7 @@
 	  && REGNO_REG_CLASS (REGNO (SET_DEST (set))) == EXTENDED_REGS
 	  && REGNO (SET_DEST (set)) != REGNO (operand)
 	  && (!past_call
-	      || !call_used_regs[REGNO (SET_DEST (set))]))
+	      || ! call_really_used_regs [REGNO (SET_DEST (set))]))
 	{
 	  rtx xoperands[2];
 	  xoperands[0] = operand;

Re: RFA: Add an FPIC set of multilibs to the MN10300 port

[Jeff Law]

Nick Clifton wrote:
> Hi Richard,
>
>> Have you tried using CALL_REALLY_USED_REGISTERS?  I.e. define
>> CALL_REALLY_USED_REGISTERS to be the same as CALL_USED_REGISTERS,
>> except that the former marks a2 as call-saved.
>
> I had not considered this (because I did not know of the existence of 
> the macro), but you are right, it is a good idea.
>
> So Jeff, Alex - here is a third version of the patch, using Richard's 
> suggestion and updating the code in mn10300.c to check the 
> call_really_used[] array rather than the call_used[] array.  Tested 
> with an mn10300-elf toolchain with no regressions in the gcc testsuite 
> and a lot of improvements for the results when compiling with -fpic.
>
> May I apply this version of the patch please ?
>
> Cheers
>   Nick
>
> gcc/ChangeLog
> 2008-10-27  Nick Clifton  <nickc@redhat.com>
>
>     * config/mn10300/mn10300.h (CALL_REALLY_USED_REGISTERS): Define.
>     * config/mn10300/mn10300.c (fp_regs_to_save): Test the
>     call_really_used_regs array rather than the call_used_regs array.
>     (mn10300_get_live_callee_saved_regs, expand_prologue,
>     expand_epilogue, output_tst): Likewise.
>
This is fine.  I wasn't aware of CALL_REALLY_USED_REGISTERS either.

jeff

Re: RFA: Add an FPIC set of multilibs to the MN10300 port

[Nick Clifton]

Hi Jeff,

>> 2008-10-27  Nick Clifton  <nickc@redhat.com>
>>
>>     * config/mn10300/mn10300.h (CALL_REALLY_USED_REGISTERS): Define.
>>     * config/mn10300/mn10300.c (fp_regs_to_save): Test the
>>     call_really_used_regs array rather than the call_used_regs array.
>>     (mn10300_get_live_callee_saved_regs, expand_prologue,
>>     expand_epilogue, output_tst): Likewise.
>>
> This is fine.  I wasn't aware of CALL_REALLY_USED_REGISTERS either.

Thanks - I have committed the patch.

Cheers
   Nick