[PATCH, m68k] Fix PR target/41302

[Maxim Kuvyrkov-2]

The attached patch fixes <gcc.gnu.org/PR41302>.

As described in the PR, the problem is in m68k_ok_for_sibcall_p()
allowing sibcalls of functions that return the result in a different
location (register A0) from what the current function expects (register D0).

This patch ports similar code from the arm backend, and also handles a
specific to m68k case when the result is returned in both D0 and A0
registers.

Tested on coldfire-elf with no regressions on gcc, g++ and libstdc++
testsuites.  OK for trunk?

Thanks,

--
Maxim Kuvyrkov
CodeSourcery
maxim@...
[phone] x724

2009-10-29  Maxim Kuvyrkov  <maxim@...>

        PR target/41302
        * config/m68k/m68k.c (m68k_ok_for_sibcall_p): Handle different
        result return locations.

2009-10-29  Maxim Kuvyrkov  <maxim@...>

        PR target/41302
        * gcc.target/m68k/pr41302.c: New test.

Index: gcc/testsuite/gcc.target/m68k/pr41302.c
===================================================================
--- gcc/testsuite/gcc.target/m68k/pr41302.c (revision 0)
+++ gcc/testsuite/gcc.target/m68k/pr41302.c (revision 0)
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+/* { dg-final { scan-assembler "move.l \%d0,\%a0" } } */
+
+struct pts {
+  int c;
+};
+
+unsigned int bar (struct pts *a, int b);
+
+struct pts * foo (struct pts *a, int b)
+{
+  return (struct pts *) bar (a, b);
+}
Index: gcc/config/m68k/m68k.c
===================================================================
--- gcc/config/m68k/m68k.c (revision 153695)
+++ gcc/config/m68k/m68k.c (working copy)
@@ -1411,6 +1411,29 @@ m68k_ok_for_sibcall_p (tree decl, tree e
   if (CALL_EXPR_STATIC_CHAIN (exp))
     return false;
 
+  if (!VOID_TYPE_P (TREE_TYPE (DECL_RESULT (cfun->decl))))
+    {
+      /* Check that the return value locations are the same.  For
+ example that we aren't returning a value from the sibling in
+ a D0 register but then need to transfer it to a A0 register.  */
+      rtx cfun_value;
+      rtx call_value;
+
+      cfun_value = m68k_function_value (TREE_TYPE (DECL_RESULT (cfun->decl)),
+ cfun->decl);
+      call_value = m68k_function_value (TREE_TYPE (exp), decl);
+
+      if ((REG_P (cfun_value)
+   && (REGNO (cfun_value) == D0_REG || REGNO (cfun_value) == A0_REG))
+  && GET_CODE (call_value) == PARALLEL)
+ /* If the current function returns the result in either D0 or A0
+   and callee function sets both these registers to the result
+   (which is equivalent to call_value being a PARALLEL in current
+   context), then the return location check passes.  */;
+      else if (!rtx_equal_p (cfun_value, call_value))
+ return false;
+    }
+
   kind = m68k_get_function_kind (current_function_decl);
   if (kind == m68k_fk_normal_function)
     /* We can always sibcall from a normal function, because it's

[Jeff Law]

On 10/30/09 04:14, Maxim Kuvyrkov wrote:

> The attached patch fixes <gcc.gnu.org/PR41302>.
>
> As described in the PR, the problem is in m68k_ok_for_sibcall_p()
> allowing sibcalls of functions that return the result in a different
> location (register A0) from what the current function expects
> (register D0).
>
> This patch ports similar code from the arm backend, and also handles a
> specific to m68k case when the result is returned in both D0 and A0
> registers.
>
> Tested on coldfire-elf with no regressions on gcc, g++ and libstdc++
> testsuites.  OK for trunk?
>
> Thanks,
>

The way you've written the condition below is somewhat odd and it's
bloody hard to see the empty statement semi-colon in the if-clause.

+      if ((REG_P (cfun_value)
+ && (REGNO (cfun_value) == D0_REG || REGNO (cfun_value) == A0_REG))
+ && GET_CODE (call_value) == PARALLEL)
+       /* If the current function returns the result in either D0 or A0
+          and callee function sets both these registers to the result
+          (which is equivalent to call_value being a PARALLEL in current
+          context), then the return location check passes.  */;
+      else if (!rtx_equal_p (cfun_value, call_value))
+       return false;
When you want an empty body, put the semi-colon on a line by itself so
that it stands out clearer.  However in this particular case, I think
you should define the conditions under which the transformation is safe
and negate that entire condition and return false in the body.



I think what you want is a condition that checks something like this

if (!(rtx_equal_p (cfun_value, call_value)
        || (GET_CODE (call_value) == PARALLEL
&& REG_P (cfun_value)
&& (REGNO (cfun_value) == D0_REG || REGNO (cfun_value == A0_REG)))

Presumably we never have a parallel which doesn't set a0/d0 together.

Jeff

[Maxim Kuvyrkov-2]

Jeff Law wrote:

> On 10/30/09 04:14, Maxim Kuvyrkov wrote:
>> The attached patch fixes <gcc.gnu.org/PR41302>.
>>
>> As described in the PR, the problem is in m68k_ok_for_sibcall_p()
>> allowing sibcalls of functions that return the result in a different
>> location (register A0) from what the current function expects
>> (register D0).
>>
>> This patch ports similar code from the arm backend, and also handles a
>> specific to m68k case when the result is returned in both D0 and A0
>> registers.
>>
>> Tested on coldfire-elf with no regressions on gcc, g++ and libstdc++
>> testsuites.  OK for trunk?
>>
>> Thanks,
>>
> The way you've written the condition below is somewhat odd and it's
> bloody hard to see the empty statement semi-colon in the if-clause.

Noted.

> I think what you want is a condition that checks something like this
>
> if (!(rtx_equal_p (cfun_value, call_value)
>        || (GET_CODE (call_value) == PARALLEL
> && REG_P (cfun_value)
> && (REGNO (cfun_value) == D0_REG || REGNO (cfun_value == A0_REG)))

Thanks, this does look more better.

>
> Presumably we never have a parallel which doesn't set a0/d0 together.

Right.

While making the changes I've spotted a couple of quirks in the patch,
and now I'm testing an updated version.  Will follow up once the tests
are finished.

Regards,

--
Maxim Kuvyrkov
CodeSourcery
maxim@...
[phone] x724

[Maxim Kuvyrkov-2]

Maxim Kuvyrkov wrote:

> Jeff Law wrote:
>> On 10/30/09 04:14, Maxim Kuvyrkov wrote:
>>> The attached patch fixes <gcc.gnu.org/PR41302>.
>>>
>>> As described in the PR, the problem is in m68k_ok_for_sibcall_p()
>>> allowing sibcalls of functions that return the result in a different
>>> location (register A0) from what the current function expects
>>> (register D0).
>>>
>>> This patch ports similar code from the arm backend, and also handles
>>> a specific to m68k case when the result is returned in both D0 and A0
>>> registers.
>>>
>>> Tested on coldfire-elf with no regressions on gcc, g++ and libstdc++
>>> testsuites.  OK for trunk?
>>>
>>> Thanks,
>>>
>> The way you've written the condition below is somewhat odd and it's
>> bloody hard to see the empty statement semi-colon in the if-clause.
>
> Noted.

...
> While making the changes I've spotted a couple of quirks in the patch,
> and now I'm testing an updated version.  Will follow up once the tests
> are finished.

Here is an updated version.  Turned out that testing on coldfire-elf was
of little relevance as ABI of cf-elf is different from cf-linux (cf-elf
always returns result in D0).  So I've fixed the patch to handle
different ABIs correctly and updated the testcase.

Tested on coldfire-linux, ok to check in?

--
Maxim Kuvyrkov
CodeSourcery
maxim@...
[phone] x724

2009-11-03  Maxim Kuvyrkov  <maxim@...>

        PR target/41302
        * config/m68k/m68k.c (m68k_reg_present_p): New static function.
        (m68k_ok_for_sibcall_p): Handle different result return locations.

2009-11-03  Maxim Kuvyrkov  <maxim@...>

        PR target/41302
        * gcc.target/m68k/pr41302.c: New test.

Index: testsuite/gcc.target/m68k/pr41302.c
===================================================================
--- testsuite/gcc.target/m68k/pr41302.c (revision 0)
+++ testsuite/gcc.target/m68k/pr41302.c (revision 0)
@@ -0,0 +1,28 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+/* { dg-final { scan-assembler "move.l \%d0,\%a0" { target *linux* } } } */
+
+struct pts {
+  int c;
+};
+
+unsigned int bar (struct pts *a, int b);
+
+struct pts * foo (struct pts *a, int b)
+{
+  return (struct pts *) bar (a, b);
+}
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+/* { dg-final { scan-assembler "move.l \%d0,\%a0" { target *linux* } } } */
+
+struct pts {
+  int c;
+};
+
+unsigned int bar (struct pts *a, int b);
+
+struct pts * foo (struct pts *a, int b)
+{
+  return (struct pts *) bar (a, b);
+}
Index: config/m68k/m68k.c
===================================================================
--- config/m68k/m68k.c (revision 153734)
+++ config/m68k/m68k.c (working copy)
@@ -1399,6 +1399,30 @@ flags_in_68881 (void)
   return cc_status.flags & CC_IN_68881;
 }
 
+/* Return true if PARALLEL contains register REGNO.  */
+static bool
+m68k_reg_present_p (const_rtx parallel, unsigned int regno)
+{
+  int i;
+
+  if (REG_P (parallel) && REGNO (parallel) == regno)
+    return true;
+
+  if (GET_CODE (parallel) != PARALLEL)
+    return false;
+
+  for (i = 0; i < XVECLEN (parallel, 0); ++i)
+    {
+      const_rtx x;
+
+      x = XEXP (XVECEXP (parallel, 0, i), 0);
+      if (REG_P (x) && REGNO (x) == regno)
+ return true;
+    }
+
+  return false;
+}
+
 /* Implement TARGET_FUNCTION_OK_FOR_SIBCALL_P.  */
 
 static bool
@@ -1411,6 +1435,26 @@ m68k_ok_for_sibcall_p (tree decl, tree e
   if (CALL_EXPR_STATIC_CHAIN (exp))
     return false;
 
+  if (!VOID_TYPE_P (TREE_TYPE (DECL_RESULT (cfun->decl))))
+    {
+      /* Check that the return value locations are the same.  For
+ example that we aren't returning a value from the sibling in
+ a D0 register but then need to transfer it to a A0 register.  */
+      rtx cfun_value;
+      rtx call_value;
+
+      cfun_value = FUNCTION_VALUE (TREE_TYPE (DECL_RESULT (cfun->decl)),
+   cfun->decl);
+      call_value = FUNCTION_VALUE (TREE_TYPE (exp), decl);
+
+      /* Check that the values are equal or that the result the callee
+ function returns is superset of what the current function returns.  */
+      if (!(rtx_equal_p (cfun_value, call_value)
+    || (REG_P (cfun_value)
+ && m68k_reg_present_p (call_value, REGNO (cfun_value)))))
+ return false;
+    }
+
   kind = m68k_get_function_kind (current_function_decl);
   if (kind == m68k_fk_normal_function)
     /* We can always sibcall from a normal function, because it's
@@ -5188,6 +5232,9 @@ m68k_libcall_value (enum machine_mode mo
   return gen_rtx_REG (mode, m68k_libcall_value_in_a0_p ? A0_REG : D0_REG);
 }
 
+/* Location in which function value is returned.
+   NOTE: Due to differences in ABIs, don't call this function directly,
+   use FUNCTION_VALUE instead.  */
 rtx
 m68k_function_value (const_tree valtype, const_tree func ATTRIBUTE_UNUSED)
 {

[Andreas Schwab-2]

Maxim Kuvyrkov <maxim@...> writes:

> 2009-11-03  Maxim Kuvyrkov  <maxim@...>
>
> PR target/41302
> * config/m68k/m68k.c (m68k_reg_present_p): New static function.
> (m68k_ok_for_sibcall_p): Handle different result return locations.
>
> 2009-11-03  Maxim Kuvyrkov  <maxim@...>
>
> PR target/41302
> * gcc.target/m68k/pr41302.c: New test.

This is ok.

Andreas.

--
Andreas Schwab, schwab@...
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."