Hi Julian,

Julian Brown <jul...@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;

Reply via email to