Committed: Fix problems with RX sibcall patterns

Committed: Fix problems with RX sibcall patterns

[Nick Clifton]

Hi Guys,

  I am checking in the patch below to fix a couple of problems with
  the RX sibcall patterns.  The first was that they were being allowed
  inside interrupt handlers, which does not work since these functions
  need special return instructions.

  The other problem was that the patterns contained a bogus USE
  statement that was confusing the label checking code.

Cheers
  Nick

gcc/ChangeLog
2009-11-26  Nick Clifton  <nickc@redhat.com>

	* config/rx/rx.c (rx_expand_epilogue): Add checks for sibcalls
	being used incorrectly.
	(rx_function_ok_for_sibcall): New function.  Do not allow indirect
	sibcalls, or sibcalls from interrupt functions.
	(TARGET_FUNCTION_OK_FOR_SIBCALL): Define.
	* config/rx/rx.md (sibcall): Convert to a define_expand.  Check
	for a MEM inside a MEM.
	(sibcall_value): Likewise.
	(sibcall_internal): New pattern containing old sibcall pattern.
	(sibcall_value_internal): Likewise.

Index: gcc/config/rx/rx.md
===================================================================
--- gcc/config/rx/rx.md	(revision 154669)
+++ gcc/config/rx/rx.md	(working copy)
@@ -388,7 +388,7 @@
 	 (match_operand:SI         1 "general_operand" "g,g"))]
   ""
   "@
-  jsr\t%A0
+  jsr\t%0
   bsr\t%A0"
   [(set_attr "length" "2,4")
    (set_attr "timings" "33")]
@@ -415,32 +415,60 @@
 	      (match_operand:SI         2 "general_operand"   "g,g")))]
   ""
   "@
-  jsr\t%A1
+  jsr\t%1
   bsr\t%A1"
   [(set_attr "length" "2,4")
    (set_attr "timings" "33")]
 )
 
-(define_insn "sibcall"
- [(call (mem:QI (match_operand:SI 0 "rx_symbolic_call_operand" "Symbol"))
-	(match_operand:SI         1 "general_operand"          "g"))
-  (return)
-  (use (match_operand             2 "" ""))]
+;; Note - we do not allow indirect sibcalls (with the address
+;; held in a register) because we cannot guarantee that the register
+;; chosen will be a call-used one.  If it is a call-saved register,
+;; then the epilogue code will corrupt it by popping the saved value
+;; off of the stack.
+(define_expand "sibcall"
+  [(parallel
+    [(call (mem:QI (match_operand:SI 0 "rx_symbolic_call_operand"))
+	   (match_operand:SI         1 "general_operand"))
+     (return)])]
   ""
+  {
+    if (MEM_P (operands[0]))
+      operands[0] = XEXP (operands[0], 0);
+  }
+)
+
+(define_insn "sibcall_internal"
+  [(call (mem:QI (match_operand:SI 0 "rx_symbolic_call_operand" "Symbol"))
+	 (match_operand:SI         1 "general_operand"          "g"))
+   (return)]
+  ""
   "bra\t%A0"
-  [(set_attr "length" "4")
+  [(set_attr "length"  "4")
    (set_attr "timings" "33")]
 )
 
-(define_insn "sibcall_value"
+(define_expand "sibcall_value"
+ [(parallel
+   [(set (match_operand                  0 "register_operand")
+	 (call (mem:QI (match_operand:SI 1 "rx_symbolic_call_operand"))
+	       (match_operand:SI         2 "general_operand")))
+    (return)])]
+  ""
+  {
+    if (MEM_P (operands[1]))
+      operands[1] = XEXP (operands[1], 0);
+  }
+)
+
+(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")))
-  (return)
-  (use (match_operand                  3 "" ""))]
+  (return)]
   ""
   "bra\t%A1"
-  [(set_attr "length" "4")
+  [(set_attr "length"  "4")
    (set_attr "timings" "33")]
 )
 
Index: gcc/config/rx/rx.c
===================================================================
--- gcc/config/rx/rx.c	(revision 154669)
+++ gcc/config/rx/rx.c	(working copy)
@@ -1190,7 +1190,7 @@
     {
       unsigned int i;
 
-      for (i = 0; i < XVECLEN (insn, 0); i++)
+      for (i = 0; i < (unsigned) XVECLEN (insn, 0); i++)
 	RTX_FRAME_RELATED_P (XVECEXP (insn, 0, i)) = 1;
     }
 }
@@ -1454,8 +1454,26 @@
   unsigned int reg;
   unsigned HOST_WIDE_INT total_size;
 
+  /* FIXME: We do not support indirect sibcalls at the moment becaause we
+     cannot guarantee that the register holding the function address is a
+     call-used register.  If it is a call-saved register then the stack
+     pop instructions generated in the epilogue will corrupt the address
+     before it is used.
+
+     Creating a new call-used-only register class works but then the
+     reload pass gets stuck because it cannot always find a call-used
+     register for spilling sibcalls.
+
+     The other possible solution is for this pass to scan forward for the
+     sibcall instruction (if it has been generated) and work out if it
+     is an indirect sibcall using a call-saved register.  If it is then
+     the address can copied into a call-used register in this epilogue
+     code and the sibcall instruction modified to use that register.  */
+
   if (is_naked_func (NULL_TREE))
     {
+      gcc_assert (! is_sibcall);
+
       /* Naked functions use their own, programmer provided epilogues.
 	 But, in order to keep gcc happy we have to generate some kind of
 	 epilogue RTL.  */
@@ -1547,9 +1565,15 @@
 	}
 
       if (is_fast_interrupt_func (NULL_TREE))
-	emit_jump_insn (gen_fast_interrupt_return ());
+	{
+	  gcc_assert (! is_sibcall);
+	  emit_jump_insn (gen_fast_interrupt_return ());
+	}
       else if (is_interrupt_func (NULL_TREE))
-	emit_jump_insn (gen_exception_return ());
+	{
+	  gcc_assert (! is_sibcall);
+	  emit_jump_insn (gen_exception_return ());
+	}
       else if (! is_sibcall)
 	emit_jump_insn (gen_simple_return ());
 
@@ -2107,6 +2131,26 @@
     &&   ! is_naked_func (decl);  
 }
 
+/* Return nonzero if it is ok to make a tail-call to DECL,
+   a function_decl or NULL if this is an indirect call, using EXP  */
+
+static bool
+rx_function_ok_for_sibcall (tree decl, tree exp)
+{
+  /* Do not allow indirect tailcalls.  The
+     sibcall patterns do not support them.  */
+  if (decl == NULL)
+    return false;
+
+  /* Never tailcall from inside interrupt handlers or naked functions.  */
+  if (is_fast_interrupt_func (NULL_TREE)
+      || is_interrupt_func (NULL_TREE)
+      || is_naked_func (NULL_TREE))
+    return false;
+
+  return true;
+}
+
 static void
 rx_file_start (void)
 {
@@ -2485,6 +2529,9 @@
 #undef  TARGET_FUNCTION_ATTRIBUTE_INLINABLE_P
 #define TARGET_FUNCTION_ATTRIBUTE_INLINABLE_P 	rx_func_attr_inlinable
 
+#undef  TARGET_FUNCTION_OK_FOR_SIBCALL
+#define TARGET_FUNCTION_OK_FOR_SIBCALL		rx_function_ok_for_sibcall
+
 #undef  TARGET_SET_CURRENT_FUNCTION
 #define TARGET_SET_CURRENT_FUNCTION		rx_set_current_function
 

Re: Committed: Fix problems with RX sibcall patterns

[Richard Henderson]

On 11/26/2009 02:44 AM, Nick Clifton wrote:
> +;; Note - we do not allow indirect sibcalls (with the address
> +;; held in a register) because we cannot guarantee that the register
> +;; chosen will be a call-used one.  If it is a call-saved register,
> +;; then the epilogue code will corrupt it by popping the saved value
> +;; off of the stack.

Note that the x86 apporach here is to define a call-clobbered register 
class.


r~

Re: Committed: Fix problems with RX sibcall patterns

[Nick Clifton]

Hi Richard,

>> +;; Note - we do not allow indirect sibcalls (with the address
>> +;; held in a register) because we cannot guarantee that the register
>> +;; chosen will be a call-used one.  If it is a call-saved register,
>> +;; then the epilogue code will corrupt it by popping the saved value
>> +;; off of the stack.
> 
> Note that the x86 apporach here is to define a call-clobbered register 
> class.

I tried that - I ran into reload spill failures which I assume were 
because there were not enough call-clobbered registers available. :-(

Cheers
   Nick

Re: Committed: Fix problems with RX sibcall patterns

[Richard Henderson]

On 12/01/2009 02:57 AM, Nick Clifton wrote:
> I tried that - I ran into reload spill failures which I assume were
> because there were not enough call-clobbered registers available. :-(

Presumably you also need to count the number of outgoing
register arguments and prevent the sibcall in that case too.
Like i386 does for regparm(3).


r~