diff mbox

PR57878, Incorrect code: live register clobbered in split2

Message ID 51E82064.8020704@redhat.com
State New
Headers show

Commit Message

Vladimir Makarov July 18, 2013, 5:05 p.m. UTC
On 07/15/2013 02:26 PM, Wei Mi wrote:
> Hi,
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57878
>
> The bug occurs because tfreq is given higher priority than bigger mode
> in reload_pseudo_compare_func. When there are multiple reload pseudos
> in the same insn, and the pseudo with bigger mode has lower thread
> frequency than other reload pseudos, it is possible the bigger mode
> pseudo cannot find available hardregs.
>
> The proposed fix is to switch the priority of bigger mode and tfreq in
> reload_pseudo_compare_func. Besides I promoted lra_assert to
> gcc_assert at the end of to make adding testcase easier since
> lra_assert will not fire on 4.8.1.
>
> bootstrap and regression test are ok on x86_64-linux-gnu. Is it ok for
> trunk and 4.8 branch?
>
> Thanks,
> Wei.
>
> 2013-07-15  Wei Mi  <wmi@google.com>
>
>         PR rtl-optimization/57878
>         * lra-assigns.c (reload_pseudo_compare_func): Switch the priority of
>         bigger mode and tfreq.
>
> 2013-07-15  Wei Mi  <wmi@google.com>
>
>         PR rtl-optimization/57878
>         * g++.dg/pr57518.C: New test.
In overall, the PR analysis and the patch is ok.  The only problem the
patch can affect generated code performance especially for 32-bit
targets.  I see regressions on 4 SPECInt2000 tests.

Here is the patch I've committed into the trunk.  It decreases the patch
effect on performance.

The patch was successfully bootstrapped an tested on x86/x86-64.

Committed to the trunk as rev. 201036.

Wei Mi, could you commit it to gcc4.8 branch.

Thanks.

2013-07-18  Vladimir Makarov  <vmakarov@redhat.com>
            Wei Mi  <wmi@google.com>

        PR rtl-optimization/57878
        * lra-assigns.c (assign_by_spills): Move non_reload_pseudos to the
        top.
        (reload_pseudo_compare_func): Check nregs first for reload
        pseudos.

2013-07-18  Wei Mi  <wmi@google.com>

        PR rtl-optimization/57878
        * g++.dg/pr57518.C: New test.

Comments

Wei Mi July 19, 2013, 8:54 p.m. UTC | #1
Thank you! backported as r201068 in gcc-4_8-branch.

Thanks,
Wei.


On Thu, Jul 18, 2013 at 10:05 AM, Vladimir Makarov <vmakarov@redhat.com> wrote:
> On 07/15/2013 02:26 PM, Wei Mi wrote:
>> Hi,
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57878
>>
>> The bug occurs because tfreq is given higher priority than bigger mode
>> in reload_pseudo_compare_func. When there are multiple reload pseudos
>> in the same insn, and the pseudo with bigger mode has lower thread
>> frequency than other reload pseudos, it is possible the bigger mode
>> pseudo cannot find available hardregs.
>>
>> The proposed fix is to switch the priority of bigger mode and tfreq in
>> reload_pseudo_compare_func. Besides I promoted lra_assert to
>> gcc_assert at the end of to make adding testcase easier since
>> lra_assert will not fire on 4.8.1.
>>
>> bootstrap and regression test are ok on x86_64-linux-gnu. Is it ok for
>> trunk and 4.8 branch?
>>
>> Thanks,
>> Wei.
>>
>> 2013-07-15  Wei Mi  <wmi@google.com>
>>
>>         PR rtl-optimization/57878
>>         * lra-assigns.c (reload_pseudo_compare_func): Switch the priority of
>>         bigger mode and tfreq.
>>
>> 2013-07-15  Wei Mi  <wmi@google.com>
>>
>>         PR rtl-optimization/57878
>>         * g++.dg/pr57518.C: New test.
> In overall, the PR analysis and the patch is ok.  The only problem the
> patch can affect generated code performance especially for 32-bit
> targets.  I see regressions on 4 SPECInt2000 tests.
>
> Here is the patch I've committed into the trunk.  It decreases the patch
> effect on performance.
>
> The patch was successfully bootstrapped an tested on x86/x86-64.
>
> Committed to the trunk as rev. 201036.
>
> Wei Mi, could you commit it to gcc4.8 branch.
>
> Thanks.
>
> 2013-07-18  Vladimir Makarov  <vmakarov@redhat.com>
>             Wei Mi  <wmi@google.com>
>
>         PR rtl-optimization/57878
>         * lra-assigns.c (assign_by_spills): Move non_reload_pseudos to the
>         top.
>         (reload_pseudo_compare_func): Check nregs first for reload
>         pseudos.
>
> 2013-07-18  Wei Mi  <wmi@google.com>
>
>         PR rtl-optimization/57878
>         * g++.dg/pr57518.C: New test.
>
diff mbox

Patch

Index: lra-assigns.c
===================================================================
--- lra-assigns.c	(revision 200959)
+++ lra-assigns.c	(working copy)
@@ -116,6 +116,11 @@  struct regno_assign_info
 /* Map regno to the corresponding regno assignment info.  */
 static struct regno_assign_info *regno_assign_info;
 
+/* All inherited, subreg or optional pseudos created before last spill
+   sub-pass.  Such pseudos are permitted to get memory instead of hard
+   regs.  */
+static bitmap_head non_reload_pseudos;
+
 /* Process a pseudo copy with execution frequency COPY_FREQ connecting
    REGNO1 and REGNO2 to form threads.  */
 static void
@@ -194,6 +199,15 @@  reload_pseudo_compare_func (const void *
   if ((diff = (ira_class_hard_regs_num[cl1]
 	       - ira_class_hard_regs_num[cl2])) != 0)
     return diff;
+  if ((diff
+       = (ira_reg_class_max_nregs[cl2][lra_reg_info[r2].biggest_mode]
+	  - ira_reg_class_max_nregs[cl1][lra_reg_info[r1].biggest_mode])) != 0
+      /* The code below executes rarely as nregs == 1 in most cases.
+	 So we should not worry about using faster data structures to
+	 check reload pseudos.  */
+      && ! bitmap_bit_p (&non_reload_pseudos, r1)
+      && ! bitmap_bit_p (&non_reload_pseudos, r2))
+    return diff;
   if ((diff = (regno_assign_info[regno_assign_info[r2].first].freq
 	       - regno_assign_info[regno_assign_info[r1].first].freq)) != 0)
     return diff;
@@ -1155,7 +1169,6 @@  assign_by_spills (void)
   rtx insn;
   basic_block bb;
   bitmap_head changed_insns, do_not_assign_nonreload_pseudos;
-  bitmap_head non_reload_pseudos;
   unsigned int u;
   bitmap_iterator bi;
   bool reload_p;
Index: testsuite/g++.dg/pr57878.C
===================================================================
--- testsuite/g++.dg/pr57878.C	(revision 0)
+++ testsuite/g++.dg/pr57878.C	(working copy)
@@ -0,0 +1,226 @@ 
+/* { dg-do compile { target i?86-*-* x86_64-*-* } } */
+/* { dg-options "-m32 -O2 -fno-omit-frame-pointer -fPIC -std=gnu++11" } */
+
+typedef int int32;
+typedef long long int64;
+typedef unsigned int uint32;
+typedef unsigned long long uint64;
+namespace std {
+  typedef unsigned int size_t;
+  template<class _CharT>
+  struct char_traits;
+  template<typename _Tp>
+  inline _Tp* __addressof(_Tp& __r) noexcept {
+    return reinterpret_cast<_Tp*> (&const_cast<char&>(reinterpret_cast<const volatile char&>(__r)));
+  }
+  template<typename _Tp>
+  struct remove_reference {
+    typedef _Tp type;
+  };
+  template<typename _Tp>
+  constexpr _Tp&& forward(typename std::remove_reference<_Tp>::type& __t) noexcept {
+    return static_cast<_Tp&&>(__t);
+  }
+}
+typedef unsigned int size_t;
+extern "C++" {
+  inline void* operator new(std::size_t, void* __p) noexcept {
+    return __p;
+  }
+}
+namespace __gnu_cxx __attribute__ ((__visibility__ ("default"))) {
+  template<typename _Tp>
+    class new_allocator {
+  public:
+    typedef size_t size_type;
+    typedef _Tp* pointer;
+  };
+}
+namespace std {
+  template<typename _Tp>
+  using __allocator_base = __gnu_cxx::new_allocator<_Tp>;
+  template<typename _Tp>
+  class allocator
+    : public __allocator_base<_Tp> {
+  public:
+    typedef size_t size_type;
+    template<typename _Tp1>
+    struct rebind {
+      typedef allocator<_Tp1> other;
+    };
+  };
+}
+namespace __gnu_cxx __attribute__ ((__visibility__ ("default"))) {
+  template<typename _CharT, typename _Traits, typename _Alloc>
+    class __sso_string_base;
+  template<typename _CharT, typename _Traits = std::char_traits<_CharT>, typename _Alloc = std::allocator<_CharT>, template <typename, typename, typename> class _Base = __sso_string_base>
+    class __versa_string;
+  template<typename _CharT, typename _Traits, typename _Alloc>
+    struct __vstring_utility {
+    typedef typename _Alloc::template rebind<_CharT>::other _CharT_alloc_type;
+    template<typename _Alloc1>
+    struct _Alloc_hider
+      : public _Alloc1 {
+      _Alloc_hider(const _Alloc1& __a, _CharT* __ptr)
+  : _Alloc1(__a), _M_p(__ptr) {
+      }
+      _CharT* _M_p;
+    };
+  };
+  template<typename _CharT, typename _Traits, typename _Alloc>
+    class __sso_string_base
+      : protected __vstring_utility<_CharT, _Traits, _Alloc> {
+    typedef __vstring_utility<_CharT, _Traits, _Alloc> _Util_Base;
+    typedef typename _Util_Base::_CharT_alloc_type _CharT_alloc_type;
+    typedef typename _CharT_alloc_type::size_type size_type;
+  private:
+    typename _Util_Base::template _Alloc_hider<_CharT_alloc_type>
+    _M_dataplus;
+    size_type _M_string_length;
+    enum {
+      _S_local_capacity = 15 };
+    union {
+      _CharT _M_local_data[_S_local_capacity + 1];
+    };
+    template<typename _InIterator>
+    void _M_construct(_InIterator __beg, _InIterator __end);
+  public:
+    size_type _M_max_size() const;
+    _CharT* _M_data() const {
+      return _M_dataplus._M_p;
+    }
+    size_type _M_length() const {
+      return _M_string_length;
+    }
+    __sso_string_base(const __sso_string_base& __rcs);
+    const _CharT_alloc_type& _M_get_allocator() const {
+    }
+  };
+  template<typename _CharT, typename _Traits, typename _Alloc>
+    __sso_string_base<_CharT, _Traits, _Alloc>:: __sso_string_base(const __sso_string_base& __rcs)
+    : _M_dataplus(__rcs._M_get_allocator(), _M_local_data) {
+    _M_construct(__rcs._M_data(), __rcs._M_data() + __rcs._M_length());
+  }
+  template<typename _CharT, typename _Traits, typename _Alloc, template <typename, typename, typename> class _Base>
+    class __versa_string
+      : private _Base<_CharT, _Traits, _Alloc> {
+  };
+}
+template<typename _CharT, typename _Traits = std::char_traits<_CharT>, typename _Alloc = std::allocator<_CharT> >
+class basic_string
+  : public __gnu_cxx::__versa_string<_CharT, _Traits, _Alloc> {
+};
+typedef basic_string<char> string;
+namespace std __attribute__ ((__visibility__ ("default"))) {
+  template<typename _Alloc, typename _Tp>
+    class __alloctr_rebind_helper {
+  public:
+    static const bool __value = true;
+  };
+  template<typename _Alloc, typename _Tp, bool = __alloctr_rebind_helper<_Alloc, _Tp>::__value>
+    struct __alloctr_rebind;
+  template<typename _Alloc, typename _Tp> struct __alloctr_rebind<_Alloc, _Tp, true>
+  {
+    typedef typename _Alloc::template rebind<_Tp>::other __type;
+  };
+  template<typename _Alloc>
+    struct allocator_traits {
+  private:
+    template<typename _Tp>
+    static typename _Tp::pointer _S_pointer_helper(_Tp*);
+    typedef decltype(_S_pointer_helper((_Alloc*)0)) __pointer;
+  public:
+    typedef __pointer pointer;
+    template<typename _Tp>
+    using rebind_alloc = typename __alloctr_rebind<_Alloc, _Tp>::__type;
+  };
+}
+namespace __gnu_cxx __attribute__ ((__visibility__ ("default"))) {
+  template<typename _Alloc> struct __alloc_traits
+    : std::allocator_traits<_Alloc>
+  {
+    typedef std::allocator_traits<_Alloc> _Base_type;
+    template<typename _Tp>
+    struct rebind {
+      typedef typename _Base_type::template rebind_alloc<_Tp>
+      other;
+    };
+  };
+}
+namespace std __attribute__ ((__visibility__ ("default"))) {
+  template<typename _T1, typename... _Args>
+    inline void _Construct(_T1* __p, _Args&&... __args) {
+    ::new(static_cast<void*>(__p)) _T1(std::forward<_Args>(__args)...);
+  }
+  template<typename _Tp, typename _Alloc>
+    struct _Vector_base {
+    typedef typename __gnu_cxx::__alloc_traits<_Alloc>::template rebind<_Tp>::other _Tp_alloc_type;
+    typedef typename __gnu_cxx::__alloc_traits<_Tp_alloc_type>::pointer pointer;
+    struct _Vector_impl
+      : public _Tp_alloc_type {
+      pointer _M_start;
+      pointer _M_finish;
+    };
+  public:
+    _Vector_impl _M_impl;
+  };
+  template<typename _Tp, typename _Alloc = std::allocator<_Tp> >
+    class vector
+      : protected _Vector_base<_Tp, _Alloc> {
+    typedef _Vector_base<_Tp, _Alloc> _Base;
+  public:
+    typedef _Tp value_type;
+    typedef typename _Base::pointer pointer;
+    typedef size_t size_type;
+    size_type size() const;
+    void push_back(const value_type& __x) {
+      _M_emplace_back_aux(__x);
+    }
+    template<typename... _Args>
+    void _M_emplace_back_aux(_Args&&... __args);
+    size_type _M_check_len();
+  };
+  template<typename _Tp, typename _Alloc> template<typename... _Args>
+    void vector<_Tp, _Alloc>:: _M_emplace_back_aux(_Args&&... __args) {
+    const size_type __len = _M_check_len();
+    pointer __new_start(static_cast<pointer>(::operator new(__len * sizeof(_Tp))));
+    pointer __new_temp(__new_start + size());
+    ::new((void *)__new_temp) _Tp(std::forward<_Args>(__args)...);
+    pointer __cur = __new_start;
+    pointer __first = this->_M_impl._M_start;
+    pointer __last = this->_M_impl._M_finish;
+    for (;
+	 __first != __last;
+	 ++__first, ++__cur) std::_Construct(std::__addressof(*__cur), *__first);
+  }
+}
+using std::vector;
+class DL {
+public:
+  struct ChunkId {
+    int64 disk_id;
+    uint64 handle;
+    uint64 version;
+    string capability;
+    ChunkId();
+  };
+  struct ChunkInfo {
+    ChunkId id;
+    uint64 mtime;
+    uint32 length;
+    int32 space_used;
+  };
+};
+class FDB {
+  void CollectChunk(const DL::ChunkInfo& chunk, const int& location);
+private:
+  struct ChunkData {
+    int location;
+    DL::ChunkInfo chunk_info;
+  };
+  vector<ChunkData> chunk_data_;
+};
+void FDB::CollectChunk(const DL::ChunkInfo& chunk, const int& location) {
+  ChunkData chunk_data;
+  chunk_data_.push_back( chunk_data);
+}