This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RFA: return_in_msb and complex-mode structures


This patch fixes one of the causes of the struct-layout-1/t024 failures
on mips-sgi-irix6.5.  The test case has a structure of the form:

    struct s { _Complex short a; };

and a function returns one of these structures by value.  When using
big-endian n32 and n64, these structures are padded at the least
significant end of the return register, as indicated by
targetm.calls.return_in_msb.

We have two different ways of coping with return_in_msb, one for BLKmode
types and one for non-BLKmode types.  The former uses the group load/store
machinery and the latter uses shifts.

The problem is with non-BLKmode types, specifically those with a complex
mode.  When compiling the function, we have the following situation:

  - FUNCTION_VALUE provides the padded return register, which is
    (reg:DI $2) in this case.

  - DECL_RTL (DECL_RESULT (cfun->decl)) is a pseudo of the same mode,
    namely DImode.

  - expand_return takes the provided return value (X, say),
    shifts it left by the appropriate amount, then copies it to
    DECL_RTL (DECL_RESULT (cfun->decl)).  The input to this shift
    is a paradoxical subreg of X.

This works fine for most modes.  Unfortunately, it doesn't work for
complex modes because the canonical form of a complex "pseudo register"
is a CONCAT of real and imaginary pseudo registers.  We can't take a
paradoxical subreg of X when X is such a CONCAT.

Similar problems happen on the caller side (i.e. calls.c).

This all suggests that return_in_msb is being handled at the wrong stage.
I based the current implementation on the promote_mode code, but in
hindsight, I think it would be better to deal with the shift as late as
possible on the callee side and as early as possible on the caller side.
We can then operate directly on the register returned by FUNCTION_VALUE.
Specifically:

  - On the caller side:
      Rather than give DECL_RTL (DECL_RESULT (cfun->decl)) the mode
      of the padded register, we should just give it the return type's
      natural mode.  It would then be up to expand_function_end to shift
      the return register left as a final step.

  - On the callee side:
      We should shift the hard register right by the appropriate amount
      before making any pseudo copies of it, then handle everything in
      the same way as !return_in_msb.

This patch only changes the handling of non-BLKmode return values.
As far as I know, the BLKmode case works fine as-is.  Since only the MIPS
and xtensa ports use return_in_msb, and since xtensa only cares about the
super-word BLKmode case, the patch should only really affect MIPS.

Although the patch fixes one of the reasons why t024 fails, another part
of it fails for more target-independent reasons.  I've therefore attached
a new testcase that failed before the patch and does pass afterwards.
Although it could be seen as unnecessary duplication, I think it's worth
having, because it's the kind of thing that just might fail at higher
optimisation levels.

Bootstrapped & regression tested on mips-sgi-irix6.5.  Also, to guard
against unintended ABI changes, I ran compat.exp with ALT_CC_UNDER_TEST
pointing to the unpatches compiler.  There were no failures.  OK to install?

Richard


	* optabs.h (force_expand_binop): Declare.
	* optabs.c (force_expand_binop): Export.
	* stmt.c (shift_return_value): Delete.
	(expand_return): Don't call it.
	* expr.h (shift_return_value): Declare.
	* calls.c (shift_returned_value): Delete in favor of...
	(shift_return_value): ...this new function.  Leave the caller to check
	for non-BLKmode values passed in the msb of a register.  Take said mode
	and a shift direction as argument.  Operate on the hard function value,
	not a pseudo.
	(expand_call): Adjust accordingly.
	* function.c (expand_function_start): If a non-BLKmode return value
	is padded at the last significant end of the return register, use the
	return value's natural mode for the DECL_RESULT, not the mode of the
	padded register.
	(expand_function_end): Shift the same sort of return values left by
	the appropriate amount.

testsuite/
	* gcc.c-torture/execute/20041118-1.c: New test.

Index: optabs.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/optabs.h,v
retrieving revision 1.40
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.40 optabs.h
*** optabs.h	9 Nov 2004 17:34:03 -0000	1.40
--- optabs.h	18 Nov 2004 11:49:58 -0000
*************** extern rtx expand_ternary_op (enum machi
*** 425,430 ****
--- 425,433 ----
  extern rtx expand_binop (enum machine_mode, optab, rtx, rtx, rtx, int,
  			 enum optab_methods);
  
+ extern bool force_expand_binop (enum machine_mode, optab, rtx, rtx, rtx, int,
+ 				enum optab_methods);
+ 
  /* Expand a binary operation with both signed and unsigned forms.  */
  extern rtx sign_expand_binop (enum machine_mode, optab, optab, rtx, rtx,
  			      rtx, int, enum optab_methods);
Index: optabs.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/optabs.c,v
retrieving revision 1.249
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.249 optabs.c
*** optabs.c	17 Nov 2004 00:56:57 -0000	1.249
--- optabs.c	18 Nov 2004 11:50:04 -0000
*************** simplify_expand_binop (enum machine_mode
*** 427,433 ****
  /* Like simplify_expand_binop, but always put the result in TARGET.
     Return true if the expansion succeeded.  */
  
! static bool
  force_expand_binop (enum machine_mode mode, optab binoptab,
  		    rtx op0, rtx op1, rtx target, int unsignedp,
  		    enum optab_methods methods)
--- 427,433 ----
  /* Like simplify_expand_binop, but always put the result in TARGET.
     Return true if the expansion succeeded.  */
  
! bool
  force_expand_binop (enum machine_mode mode, optab binoptab,
  		    rtx op0, rtx op1, rtx target, int unsignedp,
  		    enum optab_methods methods)
Index: stmt.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/stmt.c,v
retrieving revision 1.409
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.409 stmt.c
*** stmt.c	11 Nov 2004 22:02:47 -0000	1.409
--- stmt.c	18 Nov 2004 11:50:06 -0000
*************** static bool check_operand_nalternatives 
*** 110,116 ****
  static bool check_unique_operand_names (tree, tree);
  static char *resolve_operand_name_1 (char *, tree, tree);
  static void expand_null_return_1 (void);
- static rtx shift_return_value (rtx);
  static void expand_value_return (rtx);
  static void do_jump_if_equal (rtx, rtx, rtx, int);
  static int estimate_case_costs (case_node_ptr);
--- 110,115 ----
*************** expand_naked_return (void)
*** 1500,1532 ****
    emit_jump (end_label);
  }
  
- /* If the current function returns values in the most significant part
-    of a register, shift return value VAL appropriately.  The mode of
-    the function's return type is known not to be BLKmode.  */
- 
- static rtx
- shift_return_value (rtx val)
- {
-   tree type;
- 
-   type = TREE_TYPE (DECL_RESULT (current_function_decl));
-   if (targetm.calls.return_in_msb (type))
-     {
-       rtx target;
-       HOST_WIDE_INT shift;
- 
-       target = DECL_RTL (DECL_RESULT (current_function_decl));
-       shift = (GET_MODE_BITSIZE (GET_MODE (target))
- 	       - BITS_PER_UNIT * int_size_in_bytes (type));
-       if (shift > 0)
- 	val = expand_shift (LSHIFT_EXPR, GET_MODE (target),
- 			    gen_lowpart (GET_MODE (target), val),
- 			    build_int_cst (NULL_TREE, shift), target, 1);
-     }
-   return val;
- }
- 
- 
  /* Generate RTL to return from the current function, with value VAL.  */
  
  static void
--- 1499,1504 ----
*************** expand_return (tree retval)
*** 1737,1743 ****
        val = expand_expr (retval_rhs, val, GET_MODE (val), 0);
        val = force_not_mem (val);
        /* Return the calculated value.  */
!       expand_value_return (shift_return_value (val));
      }
    else
      {
--- 1709,1715 ----
        val = expand_expr (retval_rhs, val, GET_MODE (val), 0);
        val = force_not_mem (val);
        /* Return the calculated value.  */
!       expand_value_return (val);
      }
    else
      {
Index: expr.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/expr.h,v
retrieving revision 1.179
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.179 expr.h
*** expr.h	14 Nov 2004 06:22:56 -0000	1.179
--- expr.h	18 Nov 2004 11:50:07 -0000
*************** extern rtx hard_function_value (tree, tr
*** 554,559 ****
--- 554,561 ----
  
  extern rtx prepare_call_address (rtx, rtx, rtx *, int, int);
  
+ extern bool shift_return_value (enum machine_mode, bool, rtx);
+ 
  extern rtx expand_call (tree, rtx, int);
  
  extern void fixup_tail_calls (void);
Index: calls.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/calls.c,v
retrieving revision 1.370
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.370 calls.c
*** calls.c	15 Nov 2004 04:04:03 -0000	1.370
--- calls.c	18 Nov 2004 11:50:12 -0000
*************** static int check_sibcall_argument_overla
*** 147,153 ****
  
  static int combine_pending_stack_adjustment_and_call (int, struct args_size *,
  						      unsigned int);
- static bool shift_returned_value (tree, rtx *);
  static tree split_complex_values (tree);
  static tree split_complex_types (tree);
  
--- 147,152 ----
*************** check_sibcall_argument_overlap (rtx insn
*** 1715,1753 ****
    return insn != NULL_RTX;
  }
  
! /* If function value *VALUE was returned at the most significant end of a
!    register, shift it towards the least significant end and convert it to
!    TYPE's mode.  Return true and update *VALUE if some action was needed.
  
!    TYPE is the type of the function's return value, which is known not
!    to have mode BLKmode.  */
! 
! static bool
! shift_returned_value (tree type, rtx *value)
  {
!   if (targetm.calls.return_in_msb (type))
!     {
!       HOST_WIDE_INT shift;
  
!       shift = (GET_MODE_BITSIZE (GET_MODE (*value))
! 	       - BITS_PER_UNIT * int_size_in_bytes (type));
!       if (shift > 0)
! 	{
! 	  /* Shift the value into the low part of the register.  */
! 	  *value = expand_binop (GET_MODE (*value), lshr_optab, *value,
! 				 GEN_INT (shift), 0, 1, OPTAB_WIDEN);
! 
! 	  /* Truncate it to the type's mode, or its integer equivalent.
! 	     This is subject to TRULY_NOOP_TRUNCATION.  */
! 	  *value = convert_to_mode (int_mode_for_mode (TYPE_MODE (type)),
! 				    *value, 0);
! 
! 	  /* Now convert it to the final form.  */
! 	  *value = gen_lowpart (TYPE_MODE (type), *value);
! 	  return true;
! 	}
!     }
!   return false;
  }
  
  /* Remove all REG_EQUIV notes found in the insn chain.  */
--- 1714,1740 ----
    return insn != NULL_RTX;
  }
  
! /* Given that a function returns a value of mode MODE at the most
!    significant end of hard register VALUE, shift VALUE left or right
!    as specified by LEFT_P.  Return true if some action was needed.  */
  
! bool
! shift_return_value (enum machine_mode mode, bool left_p, rtx value)
  {
!   HOST_WIDE_INT shift;
  
!   gcc_assert (REG_P (value) && HARD_REGISTER_P (value));
!   shift = GET_MODE_BITSIZE (GET_MODE (value)) - GET_MODE_BITSIZE (mode);
!   if (shift == 0)
!     return false;
! 
!   /* Use ashr rather than lshr for right shifts.  This is for the benefit
!      of the MIPS port, which requires SImode values to be sign-extended
!      when stored in 64-bit registers.  */
!   if (!force_expand_binop (GET_MODE (value), left_p ? ashl_optab : ashr_optab,
! 			   value, GEN_INT (shift), value, 1, OPTAB_WIDEN))
!     gcc_unreachable ();
!   return true;
  }
  
  /* Remove all REG_EQUIV notes found in the insn chain.  */
*************** expand_call (tree exp, rtx target, int i
*** 2660,2665 ****
--- 2647,2666 ----
  		   next_arg_reg, valreg, old_inhibit_defer_pop, call_fusage,
  		   flags, & args_so_far);
  
+       /* If a non-BLKmode value is returned at the most significant end
+ 	 of a register, shift the register right by the appropriate amount
+ 	 and update VALREG accordingly.  BLKmode values are handled by the
+ 	 group load/store machinery below.  */
+       if (!structure_value_addr
+ 	  && !pcc_struct_value
+ 	  && TYPE_MODE (TREE_TYPE (exp)) != BLKmode
+ 	  && targetm.calls.return_in_msb (TREE_TYPE (exp)))
+ 	{
+ 	  if (shift_return_value (TYPE_MODE (TREE_TYPE (exp)), false, valreg))
+ 	    sibcall_failure = 1;
+ 	  valreg = gen_rtx_REG (TYPE_MODE (TREE_TYPE (exp)), REGNO (valreg));
+ 	}
+ 
        /* If call is cse'able, make appropriate pair of reg-notes around it.
  	 Test valreg so we don't crash; may safely ignore `const'
  	 if return type is void.  Disable for PARALLEL return values, because
*************** expand_call (tree exp, rtx target, int i
*** 2855,2866 ****
  	  sibcall_failure = 1;
  	}
        else
! 	{
! 	  if (shift_returned_value (TREE_TYPE (exp), &valreg))
! 	    sibcall_failure = 1;
! 
! 	  target = copy_to_reg (valreg);
! 	}
  
        if (targetm.calls.promote_function_return(funtype))
  	{
--- 2856,2862 ----
  	  sibcall_failure = 1;
  	}
        else
! 	target = copy_to_reg (valreg);
  
        if (targetm.calls.promote_function_return(funtype))
  	{
Index: function.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/function.c,v
retrieving revision 1.588
diff -c -p -F^\([(a-zA-Z0-9_]\|#define\) -r1.588 function.c
*** function.c	14 Nov 2004 06:22:56 -0000	1.588
--- function.c	18 Nov 2004 11:50:18 -0000
*************** expand_function_start (tree subr)
*** 4067,4088 ****
      {
        /* Compute the return values into a pseudo reg, which we will copy
  	 into the true return register after the cleanups are done.  */
! 
!       /* In order to figure out what mode to use for the pseudo, we
! 	 figure out what the mode of the eventual return register will
! 	 actually be, and use that.  */
!       rtx hard_reg
! 	= hard_function_value (TREE_TYPE (DECL_RESULT (subr)),
! 			       subr, 1);
! 
!       /* Structures that are returned in registers are not aggregate_value_p,
! 	 so we may see a PARALLEL or a REG.  */
!       if (REG_P (hard_reg))
! 	SET_DECL_RTL (DECL_RESULT (subr), gen_reg_rtx (GET_MODE (hard_reg)));
        else
  	{
! 	  gcc_assert (GET_CODE (hard_reg) == PARALLEL);
! 	  SET_DECL_RTL (DECL_RESULT (subr), gen_group_rtx (hard_reg));
  	}
  
        /* Set DECL_REGISTER flag so that expand_function_end will copy the
--- 4067,4097 ----
      {
        /* Compute the return values into a pseudo reg, which we will copy
  	 into the true return register after the cleanups are done.  */
!       tree return_type = TREE_TYPE (DECL_RESULT (subr));
!       if (TYPE_MODE (return_type) != BLKmode
! 	  && targetm.calls.return_in_msb (return_type))
! 	/* expand_function_end will insert the appropriate padding in
! 	   this case.  Use the return value's natural (unpadded) mode
! 	   within the function proper.  */
! 	SET_DECL_RTL (DECL_RESULT (subr),
! 		      gen_reg_rtx (TYPE_MODE (return_type)));
        else
  	{
! 	  /* In order to figure out what mode to use for the pseudo, we
! 	     figure out what the mode of the eventual return register will
! 	     actually be, and use that.  */
! 	  rtx hard_reg = hard_function_value (return_type, subr, 1);
! 
! 	  /* Structures that are returned in registers are not
! 	     aggregate_value_p, so we may see a PARALLEL or a REG.  */
! 	  if (REG_P (hard_reg))
! 	    SET_DECL_RTL (DECL_RESULT (subr),
! 			  gen_reg_rtx (GET_MODE (hard_reg)));
! 	  else
! 	    {
! 	      gcc_assert (GET_CODE (hard_reg) == PARALLEL);
! 	      SET_DECL_RTL (DECL_RESULT (subr), gen_group_rtx (hard_reg));
! 	    }
  	}
  
        /* Set DECL_REGISTER flag so that expand_function_end will copy the
*************** expand_function_end (void)
*** 4376,4385 ****
  	  if (GET_MODE (real_decl_rtl) == BLKmode)
  	    PUT_MODE (real_decl_rtl, GET_MODE (decl_rtl));
  
  	  /* If a named return value dumped decl_return to memory, then
  	     we may need to re-do the PROMOTE_MODE signed/unsigned
  	     extension.  */
! 	  if (GET_MODE (real_decl_rtl) != GET_MODE (decl_rtl))
  	    {
  	      int unsignedp = TYPE_UNSIGNED (TREE_TYPE (decl_result));
  
--- 4385,4406 ----
  	  if (GET_MODE (real_decl_rtl) == BLKmode)
  	    PUT_MODE (real_decl_rtl, GET_MODE (decl_rtl));
  
+ 	  /* If a non-BLKmode return value should be padded at the least
+ 	     significant end of the register, shift it left by the appropriate
+ 	     amount.  BLKmode results are handled using the group load/store
+ 	     machinery.  */
+ 	  if (TYPE_MODE (TREE_TYPE (decl_result)) != BLKmode
+ 	      && targetm.calls.return_in_msb (TREE_TYPE (decl_result)))
+ 	    {
+ 	      emit_move_insn (gen_rtx_REG (GET_MODE (decl_rtl),
+ 					   REGNO (real_decl_rtl)),
+ 			      decl_rtl);
+ 	      shift_return_value (GET_MODE (decl_rtl), true, real_decl_rtl);
+ 	    }
  	  /* If a named return value dumped decl_return to memory, then
  	     we may need to re-do the PROMOTE_MODE signed/unsigned
  	     extension.  */
! 	  else if (GET_MODE (real_decl_rtl) != GET_MODE (decl_rtl))
  	    {
  	      int unsignedp = TYPE_UNSIGNED (TREE_TYPE (decl_result));
  
diff -c /dev/null testsuite/gcc.c-torture/execute/20041118.c
*** /dev/null	2004-11-05 11:37:39.608355984 +0000
--- testsuite/gcc.c-torture/execute/20041118.c	2004-11-17 19:20:03.000000000 +0000
***************
*** 0 ****
--- 1,10 ----
+ struct s { _Complex unsigned short x; };
+ struct s gs = { 100 + 200i };
+ struct s __attribute__((noinline)) foo (void) { return gs; }
+ 
+ int main ()
+ {
+   if (foo ().x != gs.x)
+     abort ();
+   exit (0);
+ }


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]