Patchwork RX: Fix compilation problems when -mmax-constant-size is used

login
register
mail settings
Submitter Nick Clifton
Date Jan. 22, 2011, 4:07 p.m.
Message ID <m3mxmsudlb.fsf@redhat.com>
Download mbox | patch
Permalink /patch/80007/
State New
Headers show

Comments

Nick Clifton - Jan. 22, 2011, 4:07 p.m.
Hi Guys,

  I am applying the attached patch to fix some problems with the RX port
  when using the -mmax-constant-size feature.  This feature can be used
  to force large constants to be placed into a constant pool, instead of
  being held immediate field of instructions, which can under some
  circumstances result in better code.  But it causes problems for
  function prologues and epilogues when the stack frame is too big for
  the constant size.

Cheers
  Nick

gcc/ChangeLog
2011-01-22  Nick Clifton  <nickc@redhat.com>

	* config/rx/rx.md (UNSPEC_CONST): New.
	(deallocate_and_return): Wrap the amount popped off the stack in
	an UNSPEC_CONST in order to stop it being rejected by
	-mmax-constant-size.
	(pop_and_return): Add a "(return)" rtx.
	(call): Drop the immediate operand.
	(call_internal): Likewise.
	(call_value): Likewise.
	(call_value_internal): Likewise.
	(sibcall_internal): Likewise.
	(sibcall_value_internal): Likewise.
	(sibcall): Likewise.  Generate an explicit call using
	sibcall_internal.
	(sibcall_value): Likewise.
	(mov<>): FAIL if a constant operand is not legitimate.
	(addsi3_unpsec): New pattern.
	* config/rx/rx.c (rx_print_operand_address): Handle UNPSEC
	CONSTs.
	(ok_for_max_constant): New function.
	(gen_safe_add): New function.
	(rx_expand_prologue): Use gen_safe_add.
	(rx_expand_epilogue): Likewise.
	(rx_is_legitimate_constant): Use ok_for_max_constant.  Handle
	UNSPEC CONSTs.
Richard Henderson - Jan. 22, 2011, 8:40 p.m.
On 01/22/2011 08:07 AM, Nick Clifton wrote:
> +;; A pattern to add an integer to a register, regardless of the
> +;; setting of the -mmax-constant-size command line switch.
> +;; See rx.c:gen_safe_add() for more details.
> +(define_insn "addsi3_unspec"
> +  [(set (match_operand:SI          0 "register_operand"  "=r,r")
> +	(plus:SI (match_operand:SI 1 "register_operand"  "%0,r")
> +		 (const:SI (unspec:SI [(match_operand 2 "const_int_operand" "n,n")] UNSPEC_CONST))))
> +   (clobber (reg:CC CC_REG))]
> +  ""
> +  "@
> +  add\t%2, %0
> +  add\t%2, %1, %0"
> +  [(set_attr "timings" "11")
> +   (set_attr "length"   "6")]

You shouldn't need to do this.  Just make that CONST/UNSPEC be
a legitimate constant, and alter the print_operand routine to
handle it.


r~

Patch

Index: gcc/config/rx/rx.md
===================================================================
--- gcc/config/rx/rx.md	(revision 169049)
+++ gcc/config/rx/rx.md	(working copy)
@@ -1,5 +1,5 @@ 
 ;;  Machine Description for Renesas RX processors
-;;  Copyright (C) 2008, 2009, 2010 Free Software Foundation, Inc.
+;;  Copyright (C) 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
 ;;  Contributed by Red Hat.
 
 ;; This file is part of GCC.
@@ -50,6 +50,7 @@ 
    (UNSPEC_RTE             10)
    (UNSPEC_RTFI            11)
    (UNSPEC_NAKED           12)
+   (UNSPEC_CONST           13)
    
    (UNSPEC_MOVSTR          20)
    (UNSPEC_MOVMEM          21)
@@ -416,10 +417,12 @@ 
    (set_attr "timings" "55")]
 )
 
+;; Unspec used so that the constant will not be invalid
+;; if -mmax-constant-size has been specified.
 (define_insn "deallocate_and_return"
   [(set (reg:SI SP_REG)
 	(plus:SI (reg:SI SP_REG)
-		 (match_operand:SI 0 "immediate_operand" "i")))
+		 (const:SI (unspec:SI [(match_operand 0 "const_int_operand" "n")] UNSPEC_CONST))))
    (return)]
   ""
   "rtsd\t%0"
@@ -431,7 +434,8 @@ 
   [(match_parallel 1 "rx_rtsd_vector"
      [(set (reg:SI SP_REG)
 	   (plus:SI (reg:SI SP_REG)
-		    (match_operand:SI 0 "const_int_operand" "n")))])]
+		    (match_operand:SI 0 "const_int_operand" "n")))])
+   (return)]
   "reload_completed"
   {
     rx_emit_stack_popm (operands, false);
@@ -481,14 +485,14 @@ 
 
     if (! rx_call_operand (dest, Pmode))
       dest = force_reg (Pmode, dest);
-    emit_call_insn (gen_call_internal (dest, operands[1]));
+    emit_call_insn (gen_call_internal (dest));
     DONE;
   }
 )
 
 (define_insn "call_internal"
   [(call (mem:QI (match_operand:SI 0 "rx_call_operand" "r,Symbol"))
-	 (match_operand:SI         1 "general_operand" "g,g"))
+	 (const_int 0))
    (clobber (reg:CC CC_REG))]
   ""
   "@
@@ -508,7 +512,7 @@ 
 
     if (! rx_call_operand (dest, Pmode))
       dest = force_reg (Pmode, dest);
-    emit_call_insn (gen_call_value_internal (operands[0], dest, operands[2]));
+    emit_call_insn (gen_call_value_internal (operands[0], dest));
     DONE;
   }
 )
@@ -516,7 +520,7 @@ 
 (define_insn "call_value_internal"
   [(set (match_operand                  0 "register_operand" "=r,r")
 	(call (mem:QI (match_operand:SI 1 "rx_call_operand"   "r,Symbol"))
-	      (match_operand:SI         2 "general_operand"   "g,g")))
+	      (const_int 0)))
    (clobber (reg:CC CC_REG))]
   ""
   "@
@@ -540,12 +544,14 @@ 
   {
     if (MEM_P (operands[0]))
       operands[0] = XEXP (operands[0], 0);
+    emit_call_insn (gen_sibcall_internal (operands[0]));
+    DONE;
   }
 )
 
 (define_insn "sibcall_internal"
   [(call (mem:QI (match_operand:SI 0 "rx_symbolic_call_operand" "Symbol"))
-	 (match_operand:SI         1 "general_operand"          "g"))
+	 (const_int 0))
    (return)]
   ""
   "bra\t%A0"
@@ -563,13 +569,15 @@ 
   {
     if (MEM_P (operands[1]))
       operands[1] = XEXP (operands[1], 0);
+    emit_call_insn (gen_sibcall_value_internal (operands[0], operands[1]));
+    DONE;
   }
 )
 
 (define_insn "sibcall_value_internal"
  [(set (match_operand                  0 "register_operand"         "=r")
        (call (mem:QI (match_operand:SI 1 "rx_symbolic_call_operand" "Symbol"))
-	     (match_operand:SI         2 "general_operand"          "g")))
+	     (const_int 0)))
   (return)]
   ""
   "bra\t%A1"
@@ -621,6 +629,9 @@ 
   {
     if (MEM_P (operand0) && MEM_P (operand1))
       operands[1] = copy_to_mode_reg (<register_modes:MODE>mode, operand1);
+    if (CONST_INT_P (operand1)
+        && ! rx_is_legitimate_constant (operand1))
+      FAIL;
   }
 )
 
@@ -1110,6 +1121,22 @@ 
   DONE;
 })
 
+;; A pattern to add an integer to a register, regardless of the
+;; setting of the -mmax-constant-size command line switch.
+;; See rx.c:gen_safe_add() for more details.
+(define_insn "addsi3_unspec"
+  [(set (match_operand:SI          0 "register_operand"  "=r,r")
+	(plus:SI (match_operand:SI 1 "register_operand"  "%0,r")
+		 (const:SI (unspec:SI [(match_operand 2 "const_int_operand" "n,n")] UNSPEC_CONST))))
+   (clobber (reg:CC CC_REG))]
+  ""
+  "@
+  add\t%2, %0
+  add\t%2, %1, %0"
+  [(set_attr "timings" "11")
+   (set_attr "length"   "6")]
+)
+
 (define_insn "andsi3"
   [(set (match_operand:SI         0 "register_operand"  "=r,r,r,r,r,r,r,r,r")
 	(and:SI (match_operand:SI 1 "register_operand"  "%0,0,0,0,0,0,r,r,0")
Index: gcc/config/rx/rx.c
===================================================================
--- gcc/config/rx/rx.c	(revision 169049)
+++ gcc/config/rx/rx.c	(working copy)
@@ -1,5 +1,5 @@ 
 /* Subroutines used for code generation on Renesas RX processors.
-   Copyright (C) 2008, 2009, 2010 Free Software Foundation, Inc.
+   Copyright (C) 2008, 2009, 2010, 2011 Free Software Foundation, Inc.
    Contributed by Red Hat.
 
    This file is part of GCC.
@@ -323,10 +323,20 @@ 
 	break;
       }
 
+    case CONST:
+      if (GET_CODE (XEXP (addr, 0)) == UNSPEC)
+	{
+	  addr = XEXP (addr, 0);
+	  gcc_assert (XINT (addr, 1) == UNSPEC_CONST);
+      
+	  addr = XVECEXP (addr, 0, 0);
+	  gcc_assert (CONST_INT_P (addr));
+	}
+      /* Fall through.  */
     case LABEL_REF:
     case SYMBOL_REF:
-    case CONST:
       fprintf (file, "#");
+
     default:
       output_addr_const (file, addr);
       break;
@@ -1281,6 +1291,56 @@ 
     }
 }
 
+static bool
+ok_for_max_constant (HOST_WIDE_INT val)
+{
+  if (rx_max_constant_size == 0  || rx_max_constant_size == 4)
+    /* If there is no constraint on the size of constants
+       used as operands, then any value is legitimate.  */
+    return true;
+
+  /* rx_max_constant_size specifies the maximum number
+     of bytes that can be used to hold a signed value.  */
+  return IN_RANGE (val, (-1 << (rx_max_constant_size * 8)),
+		        ( 1 << (rx_max_constant_size * 8)));
+}
+
+/* Generate an ADD of SRC plus VAL into DEST.
+   Handles the case where VAL is too big for max_constant_value.
+   Sets FRAME_RELATED_P on the insn if IS_FRAME_RELATED is true.  */
+
+static void
+gen_safe_add (rtx dest, rtx src, rtx val, bool is_frame_related)
+{
+  rtx insn;
+
+  if (val == NULL_RTX || INTVAL (val) == 0)
+    {
+      gcc_assert (dest != src);
+
+      insn = emit_move_insn (dest, src);
+    }
+  else if (ok_for_max_constant (INTVAL (val)))
+    insn = emit_insn (gen_addsi3 (dest, src, val));
+  else
+    {
+      insn = emit_insn (gen_addsi3_unspec (dest, src, val));
+
+      if (is_frame_related)
+	/* We have to provide our own frame related note here
+	   as the dwarf2out code cannot be expected to grok
+	   our unspec.  */
+	add_reg_note (insn, REG_FRAME_RELATED_EXPR,
+		      gen_rtx_SET (SImode, dest,
+				   gen_rtx_PLUS (SImode, src, val)));
+      return;
+    }
+
+  if (is_frame_related)
+    RTX_FRAME_RELATED_P (insn) = 1;
+  return;
+}
+
 void
 rx_expand_prologue (void)
 {
@@ -1370,18 +1430,9 @@ 
 
   /* If needed, set up the frame pointer.  */
   if (frame_pointer_needed)
-    {
-      if (frame_size)
-	insn = emit_insn (gen_addsi3 (frame_pointer_rtx, stack_pointer_rtx,
-				      GEN_INT (- (HOST_WIDE_INT) frame_size)));
-      else
-	insn = emit_move_insn (frame_pointer_rtx, stack_pointer_rtx);
+    gen_safe_add (frame_pointer_rtx, stack_pointer_rtx,
+		  GEN_INT (- (HOST_WIDE_INT) frame_size), true);
 
-      RTX_FRAME_RELATED_P (insn) = 1;
-    }
-
-  insn = NULL_RTX;
-
   /* Allocate space for the outgoing args.
      If the stack frame has not already been set up then handle this as well.  */
   if (stack_size)
@@ -1389,29 +1440,26 @@ 
       if (frame_size)
 	{
 	  if (frame_pointer_needed)
-	    insn = emit_insn (gen_addsi3 (stack_pointer_rtx, frame_pointer_rtx,
-					  GEN_INT (- (HOST_WIDE_INT)
-						   stack_size)));
+	    gen_safe_add (stack_pointer_rtx, frame_pointer_rtx,
+			  GEN_INT (- (HOST_WIDE_INT) stack_size), true);
 	  else
-	    insn = emit_insn (gen_addsi3 (stack_pointer_rtx, stack_pointer_rtx,
-					  GEN_INT (- (HOST_WIDE_INT)
-						   (frame_size + stack_size))));
+	    gen_safe_add (stack_pointer_rtx, stack_pointer_rtx,
+			  GEN_INT (- (HOST_WIDE_INT) (frame_size + stack_size)),
+			  true);
 	}
       else
-	insn = emit_insn (gen_addsi3 (stack_pointer_rtx, stack_pointer_rtx,
-				      GEN_INT (- (HOST_WIDE_INT) stack_size)));
+	gen_safe_add (stack_pointer_rtx, stack_pointer_rtx,
+		      GEN_INT (- (HOST_WIDE_INT) stack_size), true);
     }
   else if (frame_size)
     {
       if (! frame_pointer_needed)
-	insn = emit_insn (gen_addsi3 (stack_pointer_rtx, stack_pointer_rtx,
-				      GEN_INT (- (HOST_WIDE_INT) frame_size)));
+	gen_safe_add (stack_pointer_rtx, stack_pointer_rtx,
+		      GEN_INT (- (HOST_WIDE_INT) frame_size), true);
       else
-	insn = emit_move_insn (stack_pointer_rtx, frame_pointer_rtx);
+	gen_safe_add (stack_pointer_rtx, frame_pointer_rtx, NULL_RTX,
+		      true);
     }
-
-  if (insn != NULL_RTX)
-    RTX_FRAME_RELATED_P (insn) = 1;
 }
 
 static void
@@ -1589,8 +1637,8 @@ 
     {
       /* Cannot use the special instructions - deconstruct by hand.  */
       if (total_size)
-	emit_insn (gen_addsi3 (stack_pointer_rtx, stack_pointer_rtx,
-			       GEN_INT (total_size)));
+	gen_safe_add (stack_pointer_rtx, stack_pointer_rtx,
+		      GEN_INT (total_size), false);
 
       if (MUST_SAVE_ACC_REGISTER)
 	{
@@ -1682,8 +1730,8 @@ 
 	  return;
 	}
 
-      emit_insn (gen_addsi3 (stack_pointer_rtx, stack_pointer_rtx,
-			     GEN_INT (total_size)));
+      gen_safe_add (stack_pointer_rtx, stack_pointer_rtx,
+		    GEN_INT (total_size), false);
     }
 
   if (low)
@@ -2342,8 +2390,6 @@ 
 bool
 rx_is_legitimate_constant (rtx x)
 {
-  HOST_WIDE_INT val;
-
   switch (GET_CODE (x))
     {
     case CONST:
@@ -2366,7 +2412,9 @@ 
 	case SYMBOL_REF:
 	  return true;
 
-	  /* One day we may have to handle UNSPEC constants here.  */
+	case UNSPEC:
+	  return XINT (x, 1) == UNSPEC_CONST;
+
 	default:
 	  /* FIXME: Can this ever happen ?  */
 	  abort ();
@@ -2386,17 +2434,7 @@ 
       break;
     }
 
-  if (rx_max_constant_size == 0  || rx_max_constant_size == 4)
-    /* If there is no constraint on the size of constants
-       used as operands, then any value is legitimate.  */
-    return true;
-
-  val = INTVAL (x);
-
-  /* rx_max_constant_size specifies the maximum number
-     of bytes that can be used to hold a signed value.  */
-  return IN_RANGE (val, (-1 << (rx_max_constant_size * 8)),
-		        ( 1 << (rx_max_constant_size * 8)));
+  return ok_for_max_constant (INTVAL (x));
 }
 
 static int