diff mbox

[ARM] Fix PR target/19599

Message ID 51937689.6030101@arm.com
State New
Headers show

Commit Message

Ramana Radhakrishnan May 15, 2013, 11:50 a.m. UTC 
Hi,

Atlast a fix for this PR which has been on my plate for ages. I've 
chosen to create a new class for all caller save registers rather than 
just sticking to maybe a single temporary register as IP_REGNUM on the 
grounds that this means we have more registers to choose from - we need 
another constraint or a special indirect sibcall pattern anyway. In any 
case keeping a call clobbered core registers class might be useful in 
other areas so it's simple enough to add this.

Bootstrapped and regression tested on A15 Linux ARM state. Applied to trunk.

regards
Ramana

2013-05-15  Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>

         PR target/19599
         * config/arm/predicates.md (call_insn_operand): New predicate.
         * config/arm/constraints.md ("Cs", "Ss"):  New constraints.
         * config/arm/arm.md (*call_insn, *call_value_insn): Match only
         if insn is not a tail call.
         (*sibcall_insn, *sibcall_value_insn): Adjust for tailcalling 
through registers.
         * config/arm/arm.h (enum reg_class): New caller save register 
class.
         (REG_CLASS_NAMES): Likewise.
         (REG_CLASS_CONTENTS): Likewise.
         * config/arm/arm.c (arm_function_ok_for_sibcall): Allow tailcalling
         without decls.

2013-05-15  Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>

         PR target/19599
         * gcc.target/arm/pr40887.c: Adjust testcase.
         * gcc.target/arm/pr19599.c: New test.

Comments

Richard Henderson May 17, 2013, 8:12 p.m. UTC | #1 
On 05/15/2013 04:50 AM, Ramana Radhakrishnan wrote:
>    /* Cannot tail-call to long calls, since these are out of range of
>       a branch instruction.  */
> -  if (arm_is_long_call_p (decl))
> +  if (decl && arm_is_long_call_p (decl))
>      return false;

You can tail call long calls via indirection now.  I.e. load the address first.

>  (define_insn "*sibcall_insn"
> - [(call (mem:SI (match_operand:SI 0 "" "X"))
> + [(call (mem:SI (match_operand:SI 0 "call_insn_operand" "Cs,Ss"))

FWIW, "i" or "s" would work just as well, given you've already constrained by
call_insn_operand; Ss isn't really needed.

> +      if (arm_arch5 || arm_arch4t)
> +	return \" bx\\t%0\\t%@ indirect register sibling call\";

Missing %?.

> +      if (arm_arch5 || arm_arch4t)
> +	return \"bx\\t%1\";

Likewise.

> diff --git a/gcc/testsuite/gcc.target/arm/pr40887.c b/gcc/testsuite/gcc.target/arm/pr40887.c
> index 0b5e873..5cabe3a 100644
> --- a/gcc/testsuite/gcc.target/arm/pr40887.c
> +++ b/gcc/testsuite/gcc.target/arm/pr40887.c
> @@ -2,9 +2,9 @@
>  /* { dg-options "-O2 -march=armv5te" }  */
>  /* { dg-final { scan-assembler "blx" } } */
>  
> -int (*indirect_func)();
> +int (*indirect_func)(int x);
>  
>  int indirect_call()
>  {
> -    return indirect_func();
> +  return indirect_func(20) + indirect_func (40);
>  }
> 

You've made this test case 100% useless.  Of course there will always be a blx
insn, since you've two calls there, and one of them will never be tail called.
 It should have been either removed or become your new test case.


r~
Ramana Radhakrishnan May 17, 2013, 10:24 p.m. UTC | #2 
On Fri, May 17, 2013 at 9:12 PM, Richard Henderson <rth@redhat.com> wrote:
> On 05/15/2013 04:50 AM, Ramana Radhakrishnan wrote:
>>    /* Cannot tail-call to long calls, since these are out of range of
>>       a branch instruction.  */
>> -  if (arm_is_long_call_p (decl))
>> +  if (decl && arm_is_long_call_p (decl))
>>      return false;
>
> You can tail call long calls via indirection now.  I.e. load the address first.

>
>>  (define_insn "*sibcall_insn"
>> - [(call (mem:SI (match_operand:SI 0 "" "X"))
>> + [(call (mem:SI (match_operand:SI 0 "call_insn_operand" "Cs,Ss"))
>
> FWIW, "i" or "s" would work just as well, given you've already constrained by
> call_insn_operand; Ss isn't really needed.

True - don't know what I was thinking there.

>
>> +      if (arm_arch5 || arm_arch4t)
>> +     return \" bx\\t%0\\t%@ indirect register sibling call\";
>
> Missing %?.

It's not marked predicable - if anything these %? markers ought to be
removed as well as I don't expect these to be conditionalized ever.

>
>> +      if (arm_arch5 || arm_arch4t)
>> +     return \"bx\\t%1\";
>
> Likewise.
>
>> diff --git a/gcc/testsuite/gcc.target/arm/pr40887.c b/gcc/testsuite/gcc.target/arm/pr40887.c
>> index 0b5e873..5cabe3a 100644
>> --- a/gcc/testsuite/gcc.target/arm/pr40887.c
>> +++ b/gcc/testsuite/gcc.target/arm/pr40887.c
>> @@ -2,9 +2,9 @@
>>  /* { dg-options "-O2 -march=armv5te" }  */
>>  /* { dg-final { scan-assembler "blx" } } */
>>
>> -int (*indirect_func)();
>> +int (*indirect_func)(int x);
>>
>>  int indirect_call()
>>  {
>> -    return indirect_func();
>> +  return indirect_func(20) + indirect_func (40);
>>  }
>>
>
> You've made this test case 100% useless.  Of course there will always be a blx
> insn, since you've two calls there, and one of them will never be tail called.
>  It should have been either removed or become your new test case.

If you haven't noticed it has effectively moved to pr19599.c as my
testcase . This is not 100% useless - this test was to make sure we
actually put out a blx for an indirect call as per PR40887 . So it
*actually* continues to test what we want as per PR40887 .

Thanks,
Ramana
>
> r~
Ramana Radhakrishnan May 22, 2013, 3:20 p.m. UTC | #3 
> This caused:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57372

A fix is forthcoming - this is a dup of PR57340.

Ramana

>
> --
> Roman
diff mbox

Patch

commit 5d0e570066009d361181be26e8ba858139ee5b8e
Author: Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
Date:   Wed May 15 08:47:20 2013 +0100

    Fix PR19599 properly.

diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index 170dcb7..036db8a 100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -5376,9 +5376,8 @@  arm_function_ok_for_sibcall (tree decl, tree exp)
   if (cfun->machine->sibcall_blocked)
     return false;
 
-  /* Never tailcall something for which we have no decl, or if we
-     are generating code for Thumb-1.  */
-  if (decl == NULL || TARGET_THUMB1)
+  /* Never tailcall something if we are generating code for Thumb-1.  */
+  if (TARGET_THUMB1)
     return false;
 
   /* The PIC register is live on entry to VxWorks PLT entries, so we
@@ -5388,13 +5387,14 @@  arm_function_ok_for_sibcall (tree decl, tree exp)
 
   /* Cannot tail-call to long calls, since these are out of range of
      a branch instruction.  */
-  if (arm_is_long_call_p (decl))
+  if (decl && arm_is_long_call_p (decl))
     return false;
 
   /* If we are interworking and the function is not declared static
      then we can't tail-call it unless we know that it exists in this
      compilation unit (since it might be a Thumb routine).  */
-  if (TARGET_INTERWORK && TREE_PUBLIC (decl) && !TREE_ASM_WRITTEN (decl))
+  if (TARGET_INTERWORK && decl && TREE_PUBLIC (decl)
+      && !TREE_ASM_WRITTEN (decl))
     return false;
 
   func_type = arm_current_func_type ();
diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
index c47fdf6..8ce8a91 100644
--- a/gcc/config/arm/arm.h
+++ b/gcc/config/arm/arm.h
@@ -1142,6 +1142,7 @@  enum reg_class
   STACK_REG,
   BASE_REGS,
   HI_REGS,
+  CALLER_SAVE_REGS,
   GENERAL_REGS,
   CORE_REGS,
   VFP_D0_D7_REGS,
@@ -1168,6 +1169,7 @@  enum reg_class
   "STACK_REG",		\
   "BASE_REGS",		\
   "HI_REGS",		\
+  "CALLER_SAVE_REGS",	\
   "GENERAL_REGS",	\
   "CORE_REGS",		\
   "VFP_D0_D7_REGS",	\
@@ -1193,6 +1195,7 @@  enum reg_class
   { 0x00002000, 0x00000000, 0x00000000, 0x00000000 }, /* STACK_REG */	\
   { 0x000020FF, 0x00000000, 0x00000000, 0x00000000 }, /* BASE_REGS */	\
   { 0x00005F00, 0x00000000, 0x00000000, 0x00000000 }, /* HI_REGS */	\
+  { 0x0000100F, 0x00000000, 0x00000000, 0x00000000 }, /* CALLER_SAVE_REGS */ \
   { 0x00005FFF, 0x00000000, 0x00000000, 0x00000000 }, /* GENERAL_REGS */ \
   { 0x00007FFF, 0x00000000, 0x00000000, 0x00000000 }, /* CORE_REGS */	\
   { 0xFFFF0000, 0x00000000, 0x00000000, 0x00000000 }, /* VFP_D0_D7_REGS  */ \
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index 06b3c18..d3bc760 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -8889,7 +8889,7 @@ 
          (match_operand 1 "" ""))
    (use (match_operand 2 "" ""))
    (clobber (reg:SI LR_REGNUM))]
-  "TARGET_ARM && arm_arch5"
+  "TARGET_ARM && arm_arch5 && !SIBLING_CALL_P (insn)"
   "blx%?\\t%0"
   [(set_attr "type" "call")]
 )
@@ -8899,7 +8899,7 @@ 
          (match_operand 1 "" ""))
    (use (match_operand 2 "" ""))
    (clobber (reg:SI LR_REGNUM))]
-  "TARGET_ARM && !arm_arch5"
+  "TARGET_ARM && !arm_arch5 && !SIBLING_CALL_P (insn)"
   "*
   return output_call (operands);
   "
@@ -8918,7 +8918,7 @@ 
 	 (match_operand 1 "" ""))
    (use (match_operand 2 "" ""))
    (clobber (reg:SI LR_REGNUM))]
-  "TARGET_ARM && !arm_arch5"
+  "TARGET_ARM && !arm_arch5 && !SIBLING_CALL_P (insn)"
   "*
   return output_call_mem (operands);
   "
@@ -8931,7 +8931,7 @@ 
 	 (match_operand 1 "" ""))
    (use (match_operand 2 "" ""))
    (clobber (reg:SI LR_REGNUM))]
-  "TARGET_THUMB1 && arm_arch5"
+  "TARGET_THUMB1 && arm_arch5 && !SIBLING_CALL_P (insn)"
   "blx\\t%0"
   [(set_attr "length" "2")
    (set_attr "type" "call")]
@@ -8942,7 +8942,7 @@ 
 	 (match_operand 1 "" ""))
    (use (match_operand 2 "" ""))
    (clobber (reg:SI LR_REGNUM))]
-  "TARGET_THUMB1 && !arm_arch5"
+  "TARGET_THUMB1 && !arm_arch5 && !SIBLING_CALL_P (insn)"
   "*
   {
     if (!TARGET_CALLER_INTERWORKING)
@@ -9001,7 +9001,7 @@ 
 	      (match_operand 2 "" "")))
    (use (match_operand 3 "" ""))
    (clobber (reg:SI LR_REGNUM))]
-  "TARGET_ARM && arm_arch5"
+  "TARGET_ARM && arm_arch5 && !SIBLING_CALL_P (insn)"
   "blx%?\\t%1"
   [(set_attr "type" "call")]
 )
@@ -9012,7 +9012,7 @@ 
 	      (match_operand 2 "" "")))
    (use (match_operand 3 "" ""))
    (clobber (reg:SI LR_REGNUM))]
-  "TARGET_ARM && !arm_arch5"
+  "TARGET_ARM && !arm_arch5 && !SIBLING_CALL_P (insn)"
   "*
   return output_call (&operands[1]);
   "
@@ -9028,7 +9028,8 @@ 
 	      (match_operand 2 "" "")))
    (use (match_operand 3 "" ""))
    (clobber (reg:SI LR_REGNUM))]
-  "TARGET_ARM && !arm_arch5 && (!CONSTANT_ADDRESS_P (XEXP (operands[1], 0)))"
+  "TARGET_ARM && !arm_arch5 && (!CONSTANT_ADDRESS_P (XEXP (operands[1], 0)))
+   && !SIBLING_CALL_P (insn)"
   "*
   return output_call_mem (&operands[1]);
   "
@@ -9078,6 +9079,7 @@ 
    (use (match_operand 2 "" ""))
    (clobber (reg:SI LR_REGNUM))]
   "TARGET_32BIT
+   && !SIBLING_CALL_P (insn)
    && (GET_CODE (operands[0]) == SYMBOL_REF)
    && !arm_is_long_call_p (SYMBOL_REF_DECL (operands[0]))"
   "*
@@ -9094,6 +9096,7 @@ 
    (use (match_operand 3 "" ""))
    (clobber (reg:SI LR_REGNUM))]
   "TARGET_32BIT
+   && !SIBLING_CALL_P (insn)
    && (GET_CODE (operands[1]) == SYMBOL_REF)
    && !arm_is_long_call_p (SYMBOL_REF_DECL (operands[1]))"
   "*
@@ -9139,6 +9142,10 @@ 
   "TARGET_32BIT"
   "
   {
+    if (!REG_P (XEXP (operands[0], 0))
+       && (GET_CODE (XEXP (operands[0], 0)) != SYMBOL_REF))
+     XEXP (operands[0], 0) = force_reg (SImode, XEXP (operands[0], 0));
+
     if (operands[2] == NULL_RTX)
       operands[2] = const0_rtx;
   }"
@@ -9153,32 +9160,52 @@ 
   "TARGET_32BIT"
   "
   {
+    if (!REG_P (XEXP (operands[1], 0)) &&
+       (GET_CODE (XEXP (operands[1],0)) != SYMBOL_REF))
+     XEXP (operands[1], 0) = force_reg (SImode, XEXP (operands[1], 0));
+
     if (operands[3] == NULL_RTX)
       operands[3] = const0_rtx;
   }"
 )
 
 (define_insn "*sibcall_insn"
- [(call (mem:SI (match_operand:SI 0 "" "X"))
+ [(call (mem:SI (match_operand:SI 0 "call_insn_operand" "Cs,Ss"))
 	(match_operand 1 "" ""))
   (return)
   (use (match_operand 2 "" ""))]
-  "TARGET_32BIT && GET_CODE (operands[0]) == SYMBOL_REF"
+  "TARGET_32BIT && SIBLING_CALL_P (insn)"
   "*
-  return NEED_PLT_RELOC ? \"b%?\\t%a0(PLT)\" : \"b%?\\t%a0\";
+  if (which_alternative == 1)
+    return NEED_PLT_RELOC ? \"b%?\\t%a0(PLT)\" : \"b%?\\t%a0\";
+  else
+    {
+      if (arm_arch5 || arm_arch4t)
+	return \" bx\\t%0\\t%@ indirect register sibling call\";
+      else
+	return \"mov%?\\t%|pc, %0\\t%@ indirect register sibling call\";
+    }
   "
   [(set_attr "type" "call")]
 )
 
 (define_insn "*sibcall_value_insn"
- [(set (match_operand 0 "" "")
-       (call (mem:SI (match_operand:SI 1 "" "X"))
+ [(set (match_operand 0 "s_register_operand" "")
+       (call (mem:SI (match_operand:SI 1 "call_insn_operand" "Cs,Ss"))
 	     (match_operand 2 "" "")))
   (return)
   (use (match_operand 3 "" ""))]
-  "TARGET_32BIT && GET_CODE (operands[1]) == SYMBOL_REF"
+  "TARGET_32BIT && SIBLING_CALL_P (insn)"
   "*
-  return NEED_PLT_RELOC ? \"b%?\\t%a1(PLT)\" : \"b%?\\t%a1\";
+  if (which_alternative == 1)
+   return NEED_PLT_RELOC ? \"b%?\\t%a1(PLT)\" : \"b%?\\t%a1\";
+  else
+    {
+      if (arm_arch5 || arm_arch4t)
+	return \"bx\\t%1\";
+      else
+	return \"mov%?\\t%|pc, %1\\t@ indirect sibling call \";
+    }
   "
   [(set_attr "type" "call")]
 )
diff --git a/gcc/config/arm/constraints.md b/gcc/config/arm/constraints.md
index 767ebfb..7e7b3e6 100644
--- a/gcc/config/arm/constraints.md
+++ b/gcc/config/arm/constraints.md
@@ -96,6 +96,9 @@ 
 (define_register_constraint "c" "CC_REG"
  "@internal The condition code register.")
 
+(define_register_constraint "Cs" "CALLER_SAVE_REGS"
+ "@internal The caller save registers.  Useful for sibcalls.")
+
 (define_constraint "I"
  "In ARM/Thumb-2 state a constant that can be used as an immediate value in a
   Data Processing instruction.  In Thumb-1 state a constant in the range
@@ -400,3 +403,9 @@ 
 ;; Additionally, we used to have a Q constraint in Thumb state, but
 ;; this wasn't really a valid memory constraint.  Again, all uses of
 ;; this now seem to have been removed.
+
+(define_constraint "Ss"
+ "@internal
+  Ss is a symbol reference."
+ (match_code "symbol_ref")
+)
diff --git a/gcc/config/arm/predicates.md b/gcc/config/arm/predicates.md
index 2e0de08..92de9fe 100644
--- a/gcc/config/arm/predicates.md
+++ b/gcc/config/arm/predicates.md
@@ -635,3 +635,7 @@ 
 (define_predicate "mem_noofs_operand"
   (and (match_code "mem")
        (match_code "reg" "0")))
+
+(define_predicate "call_insn_operand"
+  (ior (match_code "symbol_ref")
+       (match_operand 0 "s_register_operand")))
diff --git a/gcc/testsuite/gcc.target/arm/pr19599.c b/gcc/testsuite/gcc.target/arm/pr19599.c
new file mode 100644
index 0000000..e3e066c
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr19599.c
@@ -0,0 +1,10 @@ 
+/* { dg-skip-if "need at least armv5te" { *-*-* } { "-march=armv[234]*" } { "" } } */
+/* { dg-options "-O2 -march=armv5te -marm" }  */
+/* { dg-final { scan-assembler "bx" } } */
+
+int (*indirect_func)();
+
+int indirect_call()
+{
+  return indirect_func();
+}
diff --git a/gcc/testsuite/gcc.target/arm/pr40887.c b/gcc/testsuite/gcc.target/arm/pr40887.c
index 0b5e873..5cabe3a 100644
--- a/gcc/testsuite/gcc.target/arm/pr40887.c
+++ b/gcc/testsuite/gcc.target/arm/pr40887.c
@@ -2,9 +2,9 @@ 
 /* { dg-options "-O2 -march=armv5te" }  */
 /* { dg-final { scan-assembler "blx" } } */
 
-int (*indirect_func)();
+int (*indirect_func)(int x);
 
 int indirect_call()
 {
-    return indirect_func();
+  return indirect_func(20) + indirect_func (40);
 }