Patchwork ARM fixed-point support [5.5/6]: argument & return padding for libcalls

login
register
mail settings
Submitter Julian Brown
Date May 26, 2011, 4:56 p.m.
Message ID <20110526175634.4bcab9b5@rex.config>
Download mbox | patch
Permalink /patch/97597/
State New
Headers show

Comments

Julian Brown - May 26, 2011, 4:56 p.m.
This patch allows padding to be specified per-target for libcalls. This
hasn't been traditionally important, because libcalls haven't accepted
quantities which might need padding, but that's no longer true with the
new(-ish) fixed-point support helper functions.

Tested (alongside other fixed-point support patches) with cross to ARM
EABI in both big & little-endian mode (the target-specific part is to
avoid a behaviour change for half-float types on ARM). OK to apply?

Thanks,

Julian

ChangeLog

    gcc/
    * calls.c (emit_library_call_value_1): Support padding for libcall
    arguments and return values.
    * config/arm/arm.c (arm_pad_arg_upward): Pad half-float values
    downwards in big-endian mode.
Richard Earnshaw - June 30, 2011, 1:31 p.m.
On 26/05/11 17:56, Julian Brown wrote:
> This patch allows padding to be specified per-target for libcalls. This
> hasn't been traditionally important, because libcalls haven't accepted
> quantities which might need padding, but that's no longer true with the
> new(-ish) fixed-point support helper functions.
> 
> Tested (alongside other fixed-point support patches) with cross to ARM
> EABI in both big & little-endian mode (the target-specific part is to
> avoid a behaviour change for half-float types on ARM). OK to apply?
> 
> Thanks,
> 
> Julian
> 
> ChangeLog
> 
>     gcc/
>     * calls.c (emit_library_call_value_1): Support padding for libcall
>     arguments and return values.
>     * config/arm/arm.c (arm_pad_arg_upward): Pad half-float values
>     downwards in big-endian mode.
> 

OK if no generic RTL maintainer objects in the next 24 hours.

R.
H.J. Lu - Aug. 1, 2011, 1:38 p.m.
On Thu, May 26, 2011 at 9:56 AM, Julian Brown <julian@codesourcery.com> wrote:
> This patch allows padding to be specified per-target for libcalls. This
> hasn't been traditionally important, because libcalls haven't accepted
> quantities which might need padding, but that's no longer true with the
> new(-ish) fixed-point support helper functions.
>
> Tested (alongside other fixed-point support patches) with cross to ARM
> EABI in both big & little-endian mode (the target-specific part is to
> avoid a behaviour change for half-float types on ARM). OK to apply?
>
> Thanks,
>
> Julian
>
> ChangeLog
>
>    gcc/
>    * calls.c (emit_library_call_value_1): Support padding for libcall
>    arguments and return values.
>    * config/arm/arm.c (arm_pad_arg_upward): Pad half-float values
>    downwards in big-endian mode.

This breaks bootstrap on Linux/x86:

http://gcc.gnu.org/ml/gcc-regression/2011-08/msg00007.html

../../src-trunk/gcc/calls.c: In function 'rtx_def*
emit_library_call_value_1(int, rtx, rtx, libcall_type, machine_mode,
int, __va_list_tag*)':
../../src-trunk/gcc/calls.c:3832:11: error: unused variable 'size'
[-Werror=unused-variable]
cc1plus: all warnings being treated as errors

make[6]: *** [calls.o] Error 1
Richard Sandiford - Aug. 7, 2011, 5:47 p.m.
Hi Julian,

Julian Brown <julian@codesourcery.com> writes:
> This patch allows padding to be specified per-target for libcalls. This
> hasn't been traditionally important, because libcalls haven't accepted
> quantities which might need padding, but that's no longer true with the
> new(-ish) fixed-point support helper functions.
>
> Tested (alongside other fixed-point support patches) with cross to ARM
> EABI in both big & little-endian mode (the target-specific part is to
> avoid a behaviour change for half-float types on ARM). OK to apply?

This patch caused several regressions on big-endian 64-bit MIPS targets,
which now try to shift single-precision floating-point arguments to
the top of an FPR.  The calls.c part...

> diff --git a/gcc/calls.c b/gcc/calls.c
> index 44a16ff..9d5d294 100644
> --- a/gcc/calls.c
> +++ b/gcc/calls.c
> @@ -3794,13 +3794,41 @@ emit_library_call_value_1 (int retval, rtx orgfun, rtx value,
>        rtx val = argvec[argnum].value;
>        rtx reg = argvec[argnum].reg;
>        int partial = argvec[argnum].partial;
> -
> +      int size = 0;
> +      
>        /* Handle calls that pass values in multiple non-contiguous
>  	 locations.  The PA64 has examples of this for library calls.  */
>        if (reg != 0 && GET_CODE (reg) == PARALLEL)
>  	emit_group_load (reg, val, NULL_TREE, GET_MODE_SIZE (mode));
>        else if (reg != 0 && partial == 0)
> -	emit_move_insn (reg, val);
> +        {
> +	  emit_move_insn (reg, val);
> +#ifdef BLOCK_REG_PADDING
> +	  size = GET_MODE_SIZE (argvec[argnum].mode);
> +
> +	  /* Copied from load_register_parameters.  */
> +
> +	  /* Handle case where we have a value that needs shifting
> +	     up to the msb.  eg. a QImode value and we're padding
> +	     upward on a BYTES_BIG_ENDIAN machine.  */
> +	  if (size < UNITS_PER_WORD
> +	      && (argvec[argnum].locate.where_pad
> +		  == (BYTES_BIG_ENDIAN ? upward : downward)))
> +	    {
> +	      rtx x;
> +	      int shift = (UNITS_PER_WORD - size) * BITS_PER_UNIT;
> +
> +	      /* Assigning REG here rather than a temp makes CALL_FUSAGE
> +		 report the whole reg as used.  Strictly speaking, the
> +		 call only uses SIZE bytes at the msb end, but it doesn't
> +		 seem worth generating rtl to say that.  */
> +	      reg = gen_rtx_REG (word_mode, REGNO (reg));
> +	      x = expand_shift (LSHIFT_EXPR, word_mode, reg, shift, reg, 1);
> +	      if (x != reg)
> +		emit_move_insn (reg, x);
> +	    }
> +#endif
> +	}
>  
>        NO_DEFER_POP;
>      }

...looks good in itself.  The problem on MIPS is the same one that
you mention in this "???" comment:

> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index 7d52b0e..cd32fe3 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -11375,6 +11375,15 @@ arm_pad_arg_upward (enum machine_mode mode, const_tree type)
>    if (type && BYTES_BIG_ENDIAN && INTEGRAL_TYPE_P (type))
>      return false;
>  
> +  /* Half-float values are only passed to libcalls, not regular functions.
> +     They should be passed and returned as "short"s (see RTABI).  To achieve
> +     that effect in big-endian mode, pad downwards so the value is passed in
> +     the least-significant end of the register.  ??? This needs to be here
> +     rather than in arm_pad_reg_upward due to peculiarity in the handling of
> +     libcall arguments.  */
> +  if (BYTES_BIG_ENDIAN && mode == HFmode)
> +    return false;
> +

in that the value of argvec[argnum].locate.where_pad is coming from
the wrong macro: FUNCTION_ARG_PADDING rather than BLOCK_REG_PADDING.
So while the code above is conditional on BLOCK_REG_PADDING, it's
actually using the value returned by FUNCTION_ARG_PADDING instead.

This represents an ABI change.  It looks like your arm.c patch is trying
to partially undo that change for ARM, but it still affects other targets
in a similar way.

The patch below borrows the padding code from the main call routines.
It fixes the MIPS problems for me (tested on mips64-linux-gnu),
but I'm not set up for big-endian ARM testing.  From what I can tell,
other targets' BLOCK_REG_PADDING definitions already handle null types.

Richard


Index: gcc/calls.c
===================================================================
*** gcc/calls.c	2011-08-07 11:11:23.000000000 +0100
--- gcc/calls.c	2011-08-07 11:11:27.000000000 +0100
*************** emit_library_call_value_1 (int retval, r
*** 3576,3595 ****
        argvec[count].partial
  	= targetm.calls.arg_partial_bytes (args_so_far, mode, NULL_TREE, 1);
  
!       locate_and_pad_parm (mode, NULL_TREE,
  #ifdef STACK_PARMS_IN_REG_PARM_AREA
! 			   1,
  #else
! 			   argvec[count].reg != 0,
  #endif
- 			   argvec[count].partial,
- 			   NULL_TREE, &args_size, &argvec[count].locate);
- 
-       gcc_assert (!argvec[count].locate.size.var);
- 
-       if (argvec[count].reg == 0 || argvec[count].partial != 0
- 	  || reg_parm_stack_space > 0)
- 	args_size.constant += argvec[count].locate.size.constant;
  
        targetm.calls.function_arg_advance (args_so_far, mode, (tree) 0, true);
      }
--- 3576,3604 ----
        argvec[count].partial
  	= targetm.calls.arg_partial_bytes (args_so_far, mode, NULL_TREE, 1);
  
!       if (argvec[count].reg == 0
! 	  || argvec[count].partial != 0
! 	  || reg_parm_stack_space > 0)
! 	{
! 	  locate_and_pad_parm (mode, NULL_TREE,
  #ifdef STACK_PARMS_IN_REG_PARM_AREA
! 			       1,
  #else
! 			       argvec[count].reg != 0,
! #endif
! 			       argvec[count].partial,
! 			       NULL_TREE, &args_size, &argvec[count].locate);
! 	  args_size.constant += argvec[count].locate.size.constant;
! 	  gcc_assert (!argvec[count].locate.size.var);
! 	}
! #ifdef BLOCK_REG_PADDING
!       else
! 	/* The argument is passed entirely in registers.  See at which
! 	   end it should be padded.  */
! 	argvec[count].locate.where_pad =
! 	  BLOCK_REG_PADDING (mode, NULL_TREE,
! 			     GET_MODE_SIZE (mode) <= UNITS_PER_WORD);
  #endif
  
        targetm.calls.function_arg_advance (args_so_far, mode, (tree) 0, true);
      }
Index: gcc/config/arm/arm.c
===================================================================
*** gcc/config/arm/arm.c	2011-08-07 17:53:49.000000000 +0100
--- gcc/config/arm/arm.c	2011-08-07 18:38:03.000000000 +0100
*************** arm_must_pass_in_stack (enum machine_mod
*** 11432,11438 ****
     aggregate types are placed in the lowest memory address.  */
  
  bool
! arm_pad_arg_upward (enum machine_mode mode, const_tree type)
  {
    if (!TARGET_AAPCS_BASED)
      return DEFAULT_FUNCTION_ARG_PADDING(mode, type) == upward;
--- 11432,11438 ----
     aggregate types are placed in the lowest memory address.  */
  
  bool
! arm_pad_arg_upward (enum machine_mode mode ATTRIBUTE_UNUSED, const_tree type)
  {
    if (!TARGET_AAPCS_BASED)
      return DEFAULT_FUNCTION_ARG_PADDING(mode, type) == upward;
*************** arm_pad_arg_upward (enum machine_mode mo
*** 11440,11475 ****
    if (type && BYTES_BIG_ENDIAN && INTEGRAL_TYPE_P (type))
      return false;
  
-   /* Half-float values are only passed to libcalls, not regular functions.
-      They should be passed and returned as "short"s (see RTABI).  To achieve
-      that effect in big-endian mode, pad downwards so the value is passed in
-      the least-significant end of the register.  ??? This needs to be here
-      rather than in arm_pad_reg_upward due to peculiarity in the handling of
-      libcall arguments.  */
-   if (BYTES_BIG_ENDIAN && mode == HFmode)
-     return false;
- 
    return true;
  }
  
  
  /* Similarly, for use by BLOCK_REG_PADDING (MODE, TYPE, FIRST).
!    For non-AAPCS, return !BYTES_BIG_ENDIAN if the least significant
!    byte of the register has useful data, and return the opposite if the
!    most significant byte does.
!    For AAPCS, small aggregates and small complex types are always padded
!    upwards.  */
  
  bool
! arm_pad_reg_upward (enum machine_mode mode ATTRIBUTE_UNUSED,
                      tree type, int first ATTRIBUTE_UNUSED)
  {
!   if (TARGET_AAPCS_BASED
!       && BYTES_BIG_ENDIAN
!       && (AGGREGATE_TYPE_P (type) || TREE_CODE (type) == COMPLEX_TYPE
! 	  || FIXED_POINT_TYPE_P (type))
!       && int_size_in_bytes (type) <= 4)
!     return true;
  
    /* Otherwise, use default padding.  */
    return !BYTES_BIG_ENDIAN;
--- 11440,11477 ----
    if (type && BYTES_BIG_ENDIAN && INTEGRAL_TYPE_P (type))
      return false;
  
    return true;
  }
  
  
  /* Similarly, for use by BLOCK_REG_PADDING (MODE, TYPE, FIRST).
!    Return !BYTES_BIG_ENDIAN if the least significant byte of the
!    register has useful data, and return the opposite if the most
!    significant byte does.  */
  
  bool
! arm_pad_reg_upward (enum machine_mode mode,
                      tree type, int first ATTRIBUTE_UNUSED)
  {
!   if (TARGET_AAPCS_BASED && BYTES_BIG_ENDIAN)
!     {
!       /* For AAPCS, small aggregates, small fixed-point types,
! 	 and small complex types are always padded upwards.  */
!       if (type)
! 	{
! 	  if ((AGGREGATE_TYPE_P (type)
! 	       || TREE_CODE (type) == COMPLEX_TYPE
! 	       || FIXED_POINT_TYPE_P (type))
! 	      && int_size_in_bytes (type) <= 4)
! 	    return true;
! 	}
!       else
! 	{
! 	  if ((COMPLEX_MODE_P (mode) || ALL_FIXED_POINT_MODE_P (mode))
! 	      && GET_MODE_SIZE (mode) <= 4)
! 	    return true;
! 	}
!     }
  
    /* Otherwise, use default padding.  */
    return !BYTES_BIG_ENDIAN;
Andrew Pinski - Aug. 18, 2011, 7:20 p.m.
On Sun, Aug 7, 2011 at 10:47 AM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> This patch caused several regressions on big-endian 64-bit MIPS targets,
> which now try to shift single-precision floating-point arguments to
> the top of an FPR.  The calls.c part...

I reported this as bug #50113 as it is breaking bootstrap (well
causing ggc to always collect).  Can this patch be applied?

Thanks,
Andrew Pinski
Julian Brown - Aug. 24, 2011, 4:04 p.m.
On Sun, 07 Aug 2011 18:47:57 +0100
Richard Sandiford <rdsandiford@googlemail.com> wrote:

> This patch caused several regressions on big-endian 64-bit MIPS
> targets, which now try to shift single-precision floating-point
> arguments to the top of an FPR.  [...]

Sorry for the breakage!

> The patch below borrows the padding code from the main call routines.
> It fixes the MIPS problems for me (tested on mips64-linux-gnu),
> but I'm not set up for big-endian ARM testing.  From what I can tell,
> other targets' BLOCK_REG_PADDING definitions already handle null
> types.

I tested your patch very lightly on ARM, by running arm.exp &
fixed-point.exp in both big & little-endian mode, and it looks fine.
I'll set off a more-complete test run also, in case that's helpful...

Thanks,

Julian
Julian Brown - Aug. 24, 2011, 9:51 p.m.
On Wed, 24 Aug 2011 17:04:55 +0100
Julian Brown <julian@codesourcery.com> wrote:

> On Sun, 07 Aug 2011 18:47:57 +0100
> Richard Sandiford <rdsandiford@googlemail.com> wrote:
> 
> > This patch caused several regressions on big-endian 64-bit MIPS
> > targets, which now try to shift single-precision floating-point
> > arguments to the top of an FPR.  [...]
> 
> Sorry for the breakage!
> 
> > The patch below borrows the padding code from the main call
> > routines. It fixes the MIPS problems for me (tested on
> > mips64-linux-gnu), but I'm not set up for big-endian ARM testing.
> > From what I can tell, other targets' BLOCK_REG_PADDING definitions
> > already handle null types.
> 
> I tested your patch very lightly on ARM, by running arm.exp &
> fixed-point.exp in both big & little-endian mode, and it looks fine.
> I'll set off a more-complete test run also, in case that's helpful...

The patch looks fine for big/little endian, gcc/g++/libstdc++, cross to
ARM EABI, btw.

Cheers,

Julian
Richard Sandiford - Aug. 25, 2011, 5:57 p.m.
Julian Brown <julian@codesourcery.com> writes:
> On Wed, 24 Aug 2011 17:04:55 +0100
> Julian Brown <julian@codesourcery.com> wrote:
>
>> On Sun, 07 Aug 2011 18:47:57 +0100
>> Richard Sandiford <rdsandiford@googlemail.com> wrote:
>> 
>> > This patch caused several regressions on big-endian 64-bit MIPS
>> > targets, which now try to shift single-precision floating-point
>> > arguments to the top of an FPR.  [...]
>> 
>> Sorry for the breakage!
>> 
>> > The patch below borrows the padding code from the main call
>> > routines. It fixes the MIPS problems for me (tested on
>> > mips64-linux-gnu), but I'm not set up for big-endian ARM testing.
>> > From what I can tell, other targets' BLOCK_REG_PADDING definitions
>> > already handle null types.
>> 
>> I tested your patch very lightly on ARM, by running arm.exp &
>> fixed-point.exp in both big & little-endian mode, and it looks fine.
>> I'll set off a more-complete test run also, in case that's helpful...
>
> The patch looks fine for big/little endian, gcc/g++/libstdc++, cross to
> ARM EABI, btw.

Great!  Thanks for testing.

Maintainers: is the patch:

    http://gcc.gnu.org/ml/gcc-patches/2011-08/msg00735.html

OK to install?  Tested by Julian on ARM BE and LE, and by me on
mips64-linux-gnu.

Thanks,
Richard

Patch

commit e3b8b63431fc1d701ac5d3cafd19c24e6d3b5c6e
Author: Julian Brown <julian@henry8.codesourcery.com>
Date:   Thu May 26 09:06:44 2011 -0700

    Support target-specific padding for libcalls.

diff --git a/gcc/calls.c b/gcc/calls.c
index 44a16ff..9d5d294 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -3794,13 +3794,41 @@  emit_library_call_value_1 (int retval, rtx orgfun, rtx value,
       rtx val = argvec[argnum].value;
       rtx reg = argvec[argnum].reg;
       int partial = argvec[argnum].partial;
-
+      int size = 0;
+      
       /* Handle calls that pass values in multiple non-contiguous
 	 locations.  The PA64 has examples of this for library calls.  */
       if (reg != 0 && GET_CODE (reg) == PARALLEL)
 	emit_group_load (reg, val, NULL_TREE, GET_MODE_SIZE (mode));
       else if (reg != 0 && partial == 0)
-	emit_move_insn (reg, val);
+        {
+	  emit_move_insn (reg, val);
+#ifdef BLOCK_REG_PADDING
+	  size = GET_MODE_SIZE (argvec[argnum].mode);
+
+	  /* Copied from load_register_parameters.  */
+
+	  /* Handle case where we have a value that needs shifting
+	     up to the msb.  eg. a QImode value and we're padding
+	     upward on a BYTES_BIG_ENDIAN machine.  */
+	  if (size < UNITS_PER_WORD
+	      && (argvec[argnum].locate.where_pad
+		  == (BYTES_BIG_ENDIAN ? upward : downward)))
+	    {
+	      rtx x;
+	      int shift = (UNITS_PER_WORD - size) * BITS_PER_UNIT;
+
+	      /* Assigning REG here rather than a temp makes CALL_FUSAGE
+		 report the whole reg as used.  Strictly speaking, the
+		 call only uses SIZE bytes at the msb end, but it doesn't
+		 seem worth generating rtl to say that.  */
+	      reg = gen_rtx_REG (word_mode, REGNO (reg));
+	      x = expand_shift (LSHIFT_EXPR, word_mode, reg, shift, reg, 1);
+	      if (x != reg)
+		emit_move_insn (reg, x);
+	    }
+#endif
+	}
 
       NO_DEFER_POP;
     }
@@ -3866,6 +3894,15 @@  emit_library_call_value_1 (int retval, rtx orgfun, rtx value,
 	       valreg,
 	       old_inhibit_defer_pop + 1, call_fusage, flags, & args_so_far);
 
+  /* Right-shift returned value if necessary.  */
+  if (!pcc_struct_value
+      && TYPE_MODE (tfom) != BLKmode
+      && targetm.calls.return_in_msb (tfom))
+    {
+      shift_return_value (TYPE_MODE (tfom), false, valreg);
+      valreg = gen_rtx_REG (TYPE_MODE (tfom), REGNO (valreg));
+    }
+
   /* For calls to `setjmp', etc., inform function.c:setjmp_warnings
      that it should complain if nonvolatile values are live.  For
      functions that cannot return, inform flow that control does not
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 7d52b0e..cd32fe3 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -11375,6 +11375,15 @@  arm_pad_arg_upward (enum machine_mode mode, const_tree type)
   if (type && BYTES_BIG_ENDIAN && INTEGRAL_TYPE_P (type))
     return false;
 
+  /* Half-float values are only passed to libcalls, not regular functions.
+     They should be passed and returned as "short"s (see RTABI).  To achieve
+     that effect in big-endian mode, pad downwards so the value is passed in
+     the least-significant end of the register.  ??? This needs to be here
+     rather than in arm_pad_reg_upward due to peculiarity in the handling of
+     libcall arguments.  */
+  if (BYTES_BIG_ENDIAN && mode == HFmode)
+    return false;
+
   return true;
 }