[PATCH, testsuite] Fix for PR49519, miscompiled 447.dealII in SPEC CPU 2006

[PATCH, testsuite] Fix for PR49519, miscompiled 447.dealII in SPEC CPU 2006

[Kirill Yukhin]

[-- Attachment #1: Type: text/plain, Size: 799 bytes --]

Hi,
I've prepared a patch for: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49519

I've also prepared a test which reproduces the error.

ChangeLog entry:
2011-07-06  Kirill Yukhin  <kirill.yukhin@intel.com>

        PR tailcall-optimization/49519
        * calls.c (mem_overlaps_already_clobbered_arg_p): Additional
        check if address is stored in register. If so - give up.

        (check_sibcall_argument_overlap_1): Do not perform check of
        overlapping when it is call to address.

tessuite/ChangeLog entry:
2011-07-06  Kirill Yukhin  <kirill.yukhin@intel.com>

        * g++.dg/torture/pr49519.C: New test for tailcall fix.

Bootstrapped, new test fails without patch, passes when it is applied.
This fixes the bprblem with SPEC2006/447.dealII miscompile

Ok for trunk?

Thanks, K

[-- Attachment #2: pr49519.gcc.patch --]
[-- Type: application/octet-stream, Size: 3386 bytes --]

diff --git a/gcc/calls.c b/gcc/calls.c
index 7538e4e..a19d739 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -1591,6 +1591,8 @@ mem_overlaps_already_clobbered_arg_p (rtx addr, unsigned HOST_WIDE_INT size)
 	   && (XEXP (addr, 0) == crtl->args.internal_arg_pointer
 	       || XEXP (addr, 1) == crtl->args.internal_arg_pointer))
     return true;
+  else if (GET_CODE (addr) == REG)
+    return true;
   else
     return false;
 
@@ -1831,6 +1833,9 @@ check_sibcall_argument_overlap_1 (rtx x)
 
   code = GET_CODE (x);
 
+  if (code == CALL)
+    return 0;
+
   if (code == MEM)
     return mem_overlaps_already_clobbered_arg_p (XEXP (x, 0),
 						 GET_MODE_SIZE (GET_MODE (x)));
diff --git a/gcc/testsuite/g++.dg/torture/pr49519.C b/gcc/testsuite/g++.dg/torture/pr49519.C
new file mode 100644
index 0000000..2888709
--- /dev/null
+++ b/gcc/testsuite/g++.dg/torture/pr49519.C
@@ -0,0 +1,135 @@
+/* { dg-do run } */
+
+#include <stdlib.h>
+
+struct null_type {};
+
+inline const null_type cnull() { return null_type(); }
+
+template <class TT> struct cons;
+class tuple;
+
+template< int N >
+struct get_class {
+  template<class TT >
+  inline static int& get(cons<TT>& t)
+  {
+    return get_class<N-1>::template get(t.tail);
+  }
+};
+
+template<>
+struct get_class<0> {
+  template<class TT>
+  inline static int& get(cons<TT>& t)
+  {
+    return t.head;
+  }
+};
+
+template<int N, class T>
+struct element
+{
+private:
+  typedef typename T::tail_type Next;
+public:
+  typedef typename element<N-1, Next>::type type;
+};
+
+template<class T>
+struct element<0,T>
+{
+  typedef int type;
+};
+
+template<int N, class TT>
+inline int& get(cons<TT>& c) {
+  return get_class<N>::template get(c);
+}
+
+template <class TT>
+struct cons {
+  typedef TT tail_type;
+
+  int head;
+  tail_type tail;
+
+  cons() : head(), tail() {}
+
+  template <class T1, class T2, class T3, class T4>
+  cons( T1& t1, T2& t2, T3& t3, T4& t4 )
+    : head (t1),
+      tail (t2, t3, t4, cnull())
+      {}
+};
+
+template <>
+struct cons<null_type> {
+  typedef null_type tail_type;
+
+  int head;
+
+  cons() : head() {}
+
+  template<class T1>
+  cons(T1& t1, const null_type&, const null_type&, const null_type&)
+  : head (t1) {}
+};
+
+template <class T0, class T1, class T2, class T3>
+struct map_tuple_to_cons
+{
+  typedef cons<typename map_tuple_to_cons<T1, T2, T3, null_type>::type> type;
+};
+
+template <>
+struct map_tuple_to_cons<null_type, null_type, null_type, null_type>
+{
+  typedef null_type type;
+};
+
+class tuple :
+  public map_tuple_to_cons<int, int, int, int>::type
+{
+public:
+  typedef typename
+    map_tuple_to_cons<int, int, int, int>::type inherited;
+
+  tuple(const int &t0,
+        const int &t1,
+        const int &t2,
+        const int &t3)
+    : inherited(t0, t1, t2, t3) {}
+};
+
+void foo(void (*boo)(int, int, int, int), tuple t)
+{
+  boo(get<0>(t), get<1>(t), get<2>(t), get<3>(t));
+}
+
+int tailcalled_t1;
+int tailcalled_t2;
+int tailcalled_t3;
+int tailcalled_t4;
+
+void print(int t1, int t2, int t3, int t4)
+{
+  tailcalled_t1 = t1;
+  tailcalled_t2 = t2;
+  tailcalled_t3 = t3;
+  tailcalled_t4 = t4;
+}
+
+int main ()
+{
+  tuple t(1,2,3,4);
+  foo(print, t);
+
+  if( (get<0>(t) != tailcalled_t1)
+    ||(get<1>(t) != tailcalled_t2)
+    ||(get<2>(t) != tailcalled_t3)
+      ||(get<3>(t) != tailcalled_t4))
+      abort();
+
+  return 0;
+}

Re: [PATCH, testsuite] Fix for PR49519, miscompiled 447.dealII in SPEC CPU 2006

[Eric Botcazou]

> 2011-07-06  Kirill Yukhin  <kirill.yukhin@intel.com>
>
>         PR tailcall-optimization/49519

Please do not invent components, this will disable the automatic xref of the 
commit in bugzilla.  Copy the "Component" field of the PR, middle-end here.

>         * calls.c (mem_overlaps_already_clobbered_arg_p): Additional
>         check if address is stored in register. If so - give up.
>
>         (check_sibcall_argument_overlap_1): Do not perform check of
>         overlapping when it is call to address.

Are the 2 changes totally unrelated?

> Bootstrapped, new test fails without patch, passes when it is applied.
> This fixes the bprblem with SPEC2006/447.dealII miscompile
>
> Ok for trunk?

The patch lacks comments - one shouldn't need to read the PR audit trail to 
understand why the new lines are there.

-- 
Eric Botcazou

Re: [PATCH, testsuite] Fix for PR49519, miscompiled 447.dealII in SPEC CPU 2006

[Kirill Yukhin]

[-- Attachment #1: Type: text/plain, Size: 1124 bytes --]

Let me try again:
I've prepared a patch for: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49519

It fixes the problem of tailcall optimization: check for stack
overlapping was not strict enough.
Patch adds another check for clobbered stack area. If address comes
from a register - we have no idea about destination of that address.
That means we must act in conservative way - address possibly overlaps
with stack area of interest, and we should not perform tailcall
optimization

ChangeLog entry:
2011-07-06  Kirill Yukhin  <kirill.yukhin@intel.com>

       PR middle-end/49519
       * calls.c (mem_overlaps_already_clobbered_arg_p): Additional
       check if address is stored in register. If so - give up.
       (check_sibcall_argument_overlap_1): Do not perform check of
       overlapping when it is call to address.

tessuite/ChangeLog entry:
2011-07-06  Kirill Yukhin  <kirill.yukhin@intel.com>

       * g++.dg/torture/pr49519.C: New test for tailcall fix.

Bootstrapped, new test fails without patch, passes when it is applied.
This fixes the problem with SPEC2006/447.dealII miscompile

Ok for trunk?

Thanks, K

[-- Attachment #2: pr49519-1.gcc.patch --]
[-- Type: application/octet-stream, Size: 3560 bytes --]

diff --git a/gcc/calls.c b/gcc/calls.c
index 7538e4e..9ae0285 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -1591,6 +1591,10 @@ mem_overlaps_already_clobbered_arg_p (rtx addr, unsigned HOST_WIDE_INT size)
 	   && (XEXP (addr, 0) == crtl->args.internal_arg_pointer
 	       || XEXP (addr, 1) == crtl->args.internal_arg_pointer))
     return true;
+  /* If address come in register - we have no idea of its origin, so
+     give up and conservatively return true */
+  else if (GET_CODE (addr) == REG)
+    return true;
   else
     return false;
 
@@ -1831,6 +1835,10 @@ check_sibcall_argument_overlap_1 (rtx x)
 
   code = GET_CODE (x);
 
+  /* We do not check arguments of call expression */
+  if (code == CALL)
+    return 0;
+
   if (code == MEM)
     return mem_overlaps_already_clobbered_arg_p (XEXP (x, 0),
 						 GET_MODE_SIZE (GET_MODE (x)));
diff --git a/gcc/testsuite/g++.dg/torture/pr49519.C b/gcc/testsuite/g++.dg/torture/pr49519.C
new file mode 100644
index 0000000..2888709
--- /dev/null
+++ b/gcc/testsuite/g++.dg/torture/pr49519.C
@@ -0,0 +1,135 @@
+/* { dg-do run } */
+
+#include <stdlib.h>
+
+struct null_type {};
+
+inline const null_type cnull() { return null_type(); }
+
+template <class TT> struct cons;
+class tuple;
+
+template< int N >
+struct get_class {
+  template<class TT >
+  inline static int& get(cons<TT>& t)
+  {
+    return get_class<N-1>::template get(t.tail);
+  }
+};
+
+template<>
+struct get_class<0> {
+  template<class TT>
+  inline static int& get(cons<TT>& t)
+  {
+    return t.head;
+  }
+};
+
+template<int N, class T>
+struct element
+{
+private:
+  typedef typename T::tail_type Next;
+public:
+  typedef typename element<N-1, Next>::type type;
+};
+
+template<class T>
+struct element<0,T>
+{
+  typedef int type;
+};
+
+template<int N, class TT>
+inline int& get(cons<TT>& c) {
+  return get_class<N>::template get(c);
+}
+
+template <class TT>
+struct cons {
+  typedef TT tail_type;
+
+  int head;
+  tail_type tail;
+
+  cons() : head(), tail() {}
+
+  template <class T1, class T2, class T3, class T4>
+  cons( T1& t1, T2& t2, T3& t3, T4& t4 )
+    : head (t1),
+      tail (t2, t3, t4, cnull())
+      {}
+};
+
+template <>
+struct cons<null_type> {
+  typedef null_type tail_type;
+
+  int head;
+
+  cons() : head() {}
+
+  template<class T1>
+  cons(T1& t1, const null_type&, const null_type&, const null_type&)
+  : head (t1) {}
+};
+
+template <class T0, class T1, class T2, class T3>
+struct map_tuple_to_cons
+{
+  typedef cons<typename map_tuple_to_cons<T1, T2, T3, null_type>::type> type;
+};
+
+template <>
+struct map_tuple_to_cons<null_type, null_type, null_type, null_type>
+{
+  typedef null_type type;
+};
+
+class tuple :
+  public map_tuple_to_cons<int, int, int, int>::type
+{
+public:
+  typedef typename
+    map_tuple_to_cons<int, int, int, int>::type inherited;
+
+  tuple(const int &t0,
+        const int &t1,
+        const int &t2,
+        const int &t3)
+    : inherited(t0, t1, t2, t3) {}
+};
+
+void foo(void (*boo)(int, int, int, int), tuple t)
+{
+  boo(get<0>(t), get<1>(t), get<2>(t), get<3>(t));
+}
+
+int tailcalled_t1;
+int tailcalled_t2;
+int tailcalled_t3;
+int tailcalled_t4;
+
+void print(int t1, int t2, int t3, int t4)
+{
+  tailcalled_t1 = t1;
+  tailcalled_t2 = t2;
+  tailcalled_t3 = t3;
+  tailcalled_t4 = t4;
+}
+
+int main ()
+{
+  tuple t(1,2,3,4);
+  foo(print, t);
+
+  if( (get<0>(t) != tailcalled_t1)
+    ||(get<1>(t) != tailcalled_t2)
+    ||(get<2>(t) != tailcalled_t3)
+      ||(get<3>(t) != tailcalled_t4))
+      abort();
+
+  return 0;
+}

Re: [PATCH, testsuite] Fix for PR49519, miscompiled 447.dealII in SPEC CPU 2006

[Eric Botcazou]

> ChangeLog entry:
> 2011-07-06  Kirill Yukhin  <kirill.yukhin@intel.com>
>
>        PR middle-end/49519
>        * calls.c (mem_overlaps_already_clobbered_arg_p): Additional
>        check if address is stored in register. If so - give up.
>        (check_sibcall_argument_overlap_1): Do not perform check of
>        overlapping when it is call to address.
>
> tessuite/ChangeLog entry:
> 2011-07-06  Kirill Yukhin  <kirill.yukhin@intel.com>
>
>        * g++.dg/torture/pr49519.C: New test for tailcall fix.

New test is sufficient.

> Bootstrapped, new test fails without patch, passes when it is applied.
> This fixes the problem with SPEC2006/447.dealII miscompile

OK, modulo a few nits:

+  /* If address come in register - we have no idea of its origin, so
+     give up and conservatively return true */
+  else if (GET_CODE (addr) == REG)

/* If the address comes in a register, we have no idea of its origin so
   give up and conservatively return true.  */

Note the period-double-space-star-slash GNUism at the end.  Non-negotiable.


+  /* We do not check arguments of call expression */
+  if (code == CALL)
+    return 0;

Nice ambiguity, the entire machinery is about checking arguments of calls. :-)

/* We need not check the operands of the CALL expresion itself.  */


No need to retest, just make sure the changes compile (e.g. type 'make' from 
within the gcc/ directory of a bootstrap tree) and commit.

Thanks for fixing the bug.

-- 
Eric Botcazou

Re: [PATCH, testsuite] Fix for PR49519, miscompiled 447.dealII in SPEC CPU 2006

[Jakub Jelinek]

On Thu, Jul 07, 2011 at 09:52:31PM +0200, Eric Botcazou wrote:
> OK, modulo a few nits:
> 
> +  /* If address come in register - we have no idea of its origin, so
> +     give up and conservatively return true */
> +  else if (GET_CODE (addr) == REG)
> 
> /* If the address comes in a register, we have no idea of its origin so
>    give up and conservatively return true.  */
> 
> Note the period-double-space-star-slash GNUism at the end.  Non-negotiable.

Also, please use REG_P (addr) instead of GET_CODE (addr) == REG.

	Jakub

Re: [PATCH, testsuite] Fix for PR49519, miscompiled 447.dealII in SPEC CPU 2006

[Kirill Yukhin]

[-- Attachment #1: Type: text/plain, Size: 741 bytes --]

Eric, Jakub,
Thanks for your inputs, I've applied them. Updated patch is attached.

So, one more time:

ChangeLog entry:
2011-07-06  Kirill Yukhin  <kirill.yukhin@intel.com>

        PR middle-end/49519
        * calls.c (mem_overlaps_already_clobbered_arg_p): Additional
        check if address is stored in register. If so - give up.
        (check_sibcall_argument_overlap_1): Do not perform check of
        overlapping when it is call to address.

tessuite/ChangeLog entry:
2011-07-06  Kirill Yukhin  <kirill.yukhin@intel.com>

        * g++.dg/torture/pr49519.C: New test.

Bootstrapped, new test fails without the patch, passes when it is applied.
This fixes the problem with SPEC2006/447.dealII miscompile

Ok for trunk?

Thanks, K

[-- Attachment #2: pr49519-2.gcc.patch --]
[-- Type: application/octet-stream, Size: 3574 bytes --]

diff --git a/gcc/calls.c b/gcc/calls.c
index 7538e4e..0cd8cc9 100644
--- a/gcc/calls.c
+++ b/gcc/calls.c
@@ -1591,6 +1591,10 @@ mem_overlaps_already_clobbered_arg_p (rtx addr, unsigned HOST_WIDE_INT size)
 	   && (XEXP (addr, 0) == crtl->args.internal_arg_pointer
 	       || XEXP (addr, 1) == crtl->args.internal_arg_pointer))
     return true;
+  /* If the address comes in a register, we have no idea of its origin so
+     give up and conservatively return true.  */
+  else if (REG_P(addr))
+    return true;
   else
     return false;
 
@@ -1831,6 +1835,10 @@ check_sibcall_argument_overlap_1 (rtx x)
 
   code = GET_CODE (x);
 
+  /* We need not check the operands of the CALL expression itself.  */
+  if (code == CALL)
+    return 0;
+
   if (code == MEM)
     return mem_overlaps_already_clobbered_arg_p (XEXP (x, 0),
 						 GET_MODE_SIZE (GET_MODE (x)));
diff --git a/gcc/testsuite/g++.dg/torture/pr49519.C b/gcc/testsuite/g++.dg/torture/pr49519.C
new file mode 100644
index 0000000..2888709
--- /dev/null
+++ b/gcc/testsuite/g++.dg/torture/pr49519.C
@@ -0,0 +1,135 @@
+/* { dg-do run } */
+
+#include <stdlib.h>
+
+struct null_type {};
+
+inline const null_type cnull() { return null_type(); }
+
+template <class TT> struct cons;
+class tuple;
+
+template< int N >
+struct get_class {
+  template<class TT >
+  inline static int& get(cons<TT>& t)
+  {
+    return get_class<N-1>::template get(t.tail);
+  }
+};
+
+template<>
+struct get_class<0> {
+  template<class TT>
+  inline static int& get(cons<TT>& t)
+  {
+    return t.head;
+  }
+};
+
+template<int N, class T>
+struct element
+{
+private:
+  typedef typename T::tail_type Next;
+public:
+  typedef typename element<N-1, Next>::type type;
+};
+
+template<class T>
+struct element<0,T>
+{
+  typedef int type;
+};
+
+template<int N, class TT>
+inline int& get(cons<TT>& c) {
+  return get_class<N>::template get(c);
+}
+
+template <class TT>
+struct cons {
+  typedef TT tail_type;
+
+  int head;
+  tail_type tail;
+
+  cons() : head(), tail() {}
+
+  template <class T1, class T2, class T3, class T4>
+  cons( T1& t1, T2& t2, T3& t3, T4& t4 )
+    : head (t1),
+      tail (t2, t3, t4, cnull())
+      {}
+};
+
+template <>
+struct cons<null_type> {
+  typedef null_type tail_type;
+
+  int head;
+
+  cons() : head() {}
+
+  template<class T1>
+  cons(T1& t1, const null_type&, const null_type&, const null_type&)
+  : head (t1) {}
+};
+
+template <class T0, class T1, class T2, class T3>
+struct map_tuple_to_cons
+{
+  typedef cons<typename map_tuple_to_cons<T1, T2, T3, null_type>::type> type;
+};
+
+template <>
+struct map_tuple_to_cons<null_type, null_type, null_type, null_type>
+{
+  typedef null_type type;
+};
+
+class tuple :
+  public map_tuple_to_cons<int, int, int, int>::type
+{
+public:
+  typedef typename
+    map_tuple_to_cons<int, int, int, int>::type inherited;
+
+  tuple(const int &t0,
+        const int &t1,
+        const int &t2,
+        const int &t3)
+    : inherited(t0, t1, t2, t3) {}
+};
+
+void foo(void (*boo)(int, int, int, int), tuple t)
+{
+  boo(get<0>(t), get<1>(t), get<2>(t), get<3>(t));
+}
+
+int tailcalled_t1;
+int tailcalled_t2;
+int tailcalled_t3;
+int tailcalled_t4;
+
+void print(int t1, int t2, int t3, int t4)
+{
+  tailcalled_t1 = t1;
+  tailcalled_t2 = t2;
+  tailcalled_t3 = t3;
+  tailcalled_t4 = t4;
+}
+
+int main ()
+{
+  tuple t(1,2,3,4);
+  foo(print, t);
+
+  if( (get<0>(t) != tailcalled_t1)
+    ||(get<1>(t) != tailcalled_t2)
+    ||(get<2>(t) != tailcalled_t3)
+      ||(get<3>(t) != tailcalled_t4))
+      abort();
+
+  return 0;
+}

Re: [PATCH, testsuite] Fix for PR49519, miscompiled 447.dealII in SPEC CPU 2006

[Eric Botcazou]

> So, one more time:

You didn't really need to re-submit but...

> ChangeLog entry:
> 2011-07-06  Kirill Yukhin  <kirill.yukhin@intel.com>
>
>         PR middle-end/49519
>         * calls.c (mem_overlaps_already_clobbered_arg_p): Additional
>         check if address is stored in register. If so - give up.
>         (check_sibcall_argument_overlap_1): Do not perform check of
>         overlapping when it is call to address.
>
> tessuite/ChangeLog entry:
> 2011-07-06  Kirill Yukhin  <kirill.yukhin@intel.com>
>
>         * g++.dg/torture/pr49519.C: New test.

OK, thanks.

-- 
Eric Botcazou

Re: [PATCH, testsuite] Fix for PR49519, miscompiled 447.dealII in SPEC CPU 2006

[H.J. Lu]

On Fri, Jul 8, 2011 at 3:58 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> So, one more time:
>
> You didn't really need to re-submit but...
>
>> ChangeLog entry:
>> 2011-07-06  Kirill Yukhin  <kirill.yukhin@intel.com>
>>
>>         PR middle-end/49519
>>         * calls.c (mem_overlaps_already_clobbered_arg_p): Additional
>>         check if address is stored in register. If so - give up.
>>         (check_sibcall_argument_overlap_1): Do not perform check of
>>         overlapping when it is call to address.
>>
>> tessuite/ChangeLog entry:
>> 2011-07-06  Kirill Yukhin  <kirill.yukhin@intel.com>
>>
>>         * g++.dg/torture/pr49519.C: New test.
>
> OK, thanks.
>

I checked it in for Kirill.

Thanks.

-- 
H.J.