| Message ID | 20110526175634.4bcab9b5@rex.config |
|---|---|
| State | New |
| Headers | show |
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.
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
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;
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
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
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
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
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;
}
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.