From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19790 invoked by alias); 7 Aug 2011 17:48:18 -0000 Received: (qmail 19780 invoked by uid 22791); 7 Aug 2011 17:48:16 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-ww0-f51.google.com (HELO mail-ww0-f51.google.com) (74.125.82.51) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 07 Aug 2011 17:48:01 +0000 Received: by wwi18 with SMTP id 18so3581232wwi.8 for ; Sun, 07 Aug 2011 10:48:00 -0700 (PDT) Received: by 10.227.19.204 with SMTP id c12mr3770167wbb.114.1312739280322; Sun, 07 Aug 2011 10:48:00 -0700 (PDT) Received: from localhost (rsandifo.gotadsl.co.uk [82.133.89.107]) by mx.google.com with ESMTPS id em16sm3879629wbb.67.2011.08.07.10.47.58 (version=TLSv1/SSLv3 cipher=OTHER); Sun, 07 Aug 2011 10:47:59 -0700 (PDT) From: Richard Sandiford To: Julian Brown Mail-Followup-To: Julian Brown ,gcc-patches@gcc.gnu.org, rdsandiford@googlemail.com Cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] ARM fixed-point support [5.5/6]: argument & return padding for libcalls References: <20110526175634.4bcab9b5@rex.config> Date: Sun, 07 Aug 2011 18:50:00 -0000 In-Reply-To: <20110526175634.4bcab9b5@rex.config> (Julian Brown's message of "Thu, 26 May 2011 17:56:34 +0100") Message-ID: <87sjpdjepe.fsf@firetop.home> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2011-08/txt/msg00737.txt.bz2 Hi Julian, Julian Brown 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;