[2/3] Fix autoprofiledbootstrap

Message ID 20190114081942.9088-2-andi@firstfloor.org
State New
Headers show
Series
  • [1/3] Lower sampling rate for autofdo bootstrap
Related show

Commit Message

Andi Kleen Jan. 14, 2019, 8:19 a.m.
From: Andi Kleen <ak@linux.intel.com>


autoprofiledbootstrap fails currently with

In file included from ../../gcc/gcc/hash-table.h:236,
                 from ../../gcc/gcc/coretypes.h:440,
                 from ../../gcc/gcc/ipa-devirt.c:110:
In static member function 'static void va_heap::release(vec<T, va_heap, vl_embed>*&) [with T = tree_node*]',
    inlined from 'void vec<T>::release() [with T = tree_node*]' at ../../gcc/gcc/vec.h:1679:20,
    inlined from 'auto_vec<T, N>::~auto_vec() [with T = tree_node*; long unsigned int N = 8]' at ../../gcc/gcc/vec.h:1436:5,
    inlined from 'vec<cgraph_node*> possible_polymorphic_call_targets(tree, long int, ipa_polymorphic_call_context, bool*, void**, bool)' at ../../gcc/gcc/ipa-devirt.c:3099:22:
../../gcc/gcc/vec.h:311:10: error: attempt to free a non-heap object 'bases_to_consider' [-Werror=free-nonheap-object]
  311 |   ::free (v);
      |   ~~~~~~~^~~
../../gcc/gcc/vec.h:311:10: error: attempt to free a non-heap object 'bases_to_consider' [-Werror=free-nonheap-object]
cc1plus: all warnings being treated as errors

The problem is that auto_vec uses a variable to keep track if the vector
is on the heap or auto. Normally this gets constant resolved, but only
when the right functions are inlined. With autofdo for some reason
the compiler decides to not inline these vec functions, even though
they are marked as "inline"

Mark them as ALWAYS_INLINE instead.

gcc/:

2019-01-14  Andi Kleen  <ak@linux.intel.com>

	* vec.h (using_auto_storage, release): Mark as ALWAYS_INLINE.
---
 gcc/vec.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.19.1

Comments

Richard Biener Jan. 14, 2019, 9:04 a.m. | #1
On Mon, Jan 14, 2019 at 9:20 AM Andi Kleen <andi@firstfloor.org> wrote:
>

> From: Andi Kleen <ak@linux.intel.com>

>

> autoprofiledbootstrap fails currently with

>

> In file included from ../../gcc/gcc/hash-table.h:236,

>                  from ../../gcc/gcc/coretypes.h:440,

>                  from ../../gcc/gcc/ipa-devirt.c:110:

> In static member function 'static void va_heap::release(vec<T, va_heap, vl_embed>*&) [with T = tree_node*]',

>     inlined from 'void vec<T>::release() [with T = tree_node*]' at ../../gcc/gcc/vec.h:1679:20,

>     inlined from 'auto_vec<T, N>::~auto_vec() [with T = tree_node*; long unsigned int N = 8]' at ../../gcc/gcc/vec.h:1436:5,

>     inlined from 'vec<cgraph_node*> possible_polymorphic_call_targets(tree, long int, ipa_polymorphic_call_context, bool*, void**, bool)' at ../../gcc/gcc/ipa-devirt.c:3099:22:

> ../../gcc/gcc/vec.h:311:10: error: attempt to free a non-heap object 'bases_to_consider' [-Werror=free-nonheap-object]

>   311 |   ::free (v);

>       |   ~~~~~~~^~~

> ../../gcc/gcc/vec.h:311:10: error: attempt to free a non-heap object 'bases_to_consider' [-Werror=free-nonheap-object]

> cc1plus: all warnings being treated as errors

>

> The problem is that auto_vec uses a variable to keep track if the vector

> is on the heap or auto. Normally this gets constant resolved, but only

> when the right functions are inlined. With autofdo for some reason

> the compiler decides to not inline these vec functions, even though

> they are marked as "inline"

>

> Mark them as ALWAYS_INLINE instead.


This might fix your case but I think it only papers over the issue.  Consider

 auto_vec<...> vec;
 not-inlined-foo (vec);

where the function can end up re-allocating the vector.  I think the more
appropriate fix is to add #pragma GCC diagnostic pus/pop and
ignored "-Wfree-nonheap-object" around the inline function (and hope
for the best that works in the inlined contexts...)

Richard.

> gcc/:

>

> 2019-01-14  Andi Kleen  <ak@linux.intel.com>

>

>         * vec.h (using_auto_storage, release): Mark as ALWAYS_INLINE.

> ---

>  gcc/vec.h | 4 ++--

>  1 file changed, 2 insertions(+), 2 deletions(-)

>

> diff --git a/gcc/vec.h b/gcc/vec.h

> index 407269c5ad3..1f5b78b1fac 100644

> --- a/gcc/vec.h

> +++ b/gcc/vec.h

> @@ -1664,7 +1664,7 @@ vec<T, va_heap, vl_ptr>::create (unsigned nelems MEM_STAT_DECL)

>  /* Free the memory occupied by the embedded vector.  */

>

>  template<typename T>

> -inline void

> +ALWAYS_INLINE void

>  vec<T, va_heap, vl_ptr>::release (void)

>  {

>    if (!m_vec)

> @@ -1940,7 +1940,7 @@ vec<T, va_heap, vl_ptr>::reverse (void)

>  }

>

>  template<typename T>

> -inline bool

> +ALWAYS_INLINE bool

>  vec<T, va_heap, vl_ptr>::using_auto_storage () const

>  {

>    return m_vec->m_vecpfx.m_using_auto_storage;

> --

> 2.19.1

>
Bin.Cheng Jan. 14, 2019, 9:11 a.m. | #2
On Mon, Jan 14, 2019 at 4:20 PM Andi Kleen <andi@firstfloor.org> wrote:
>

> From: Andi Kleen <ak@linux.intel.com>

>

> autoprofiledbootstrap fails currently with

>

> In file included from ../../gcc/gcc/hash-table.h:236,

>                  from ../../gcc/gcc/coretypes.h:440,

>                  from ../../gcc/gcc/ipa-devirt.c:110:

> In static member function 'static void va_heap::release(vec<T, va_heap, vl_embed>*&) [with T = tree_node*]',

>     inlined from 'void vec<T>::release() [with T = tree_node*]' at ../../gcc/gcc/vec.h:1679:20,

>     inlined from 'auto_vec<T, N>::~auto_vec() [with T = tree_node*; long unsigned int N = 8]' at ../../gcc/gcc/vec.h:1436:5,

>     inlined from 'vec<cgraph_node*> possible_polymorphic_call_targets(tree, long int, ipa_polymorphic_call_context, bool*, void**, bool)' at ../../gcc/gcc/ipa-devirt.c:3099:22:

> ../../gcc/gcc/vec.h:311:10: error: attempt to free a non-heap object 'bases_to_consider' [-Werror=free-nonheap-object]

>   311 |   ::free (v);

>       |   ~~~~~~~^~~

> ../../gcc/gcc/vec.h:311:10: error: attempt to free a non-heap object 'bases_to_consider' [-Werror=free-nonheap-object]

> cc1plus: all warnings being treated as errors

>

> The problem is that auto_vec uses a variable to keep track if the vector

> is on the heap or auto. Normally this gets constant resolved, but only

> when the right functions are inlined. With autofdo for some reason

> the compiler decides to not inline these vec functions, even though

> they are marked as "inline"

A comment not closely related to this patch.  We observed the same
inline behavior in which perf data is inadequate, sometime it has
non-trivial impact on kernel compilation.  We have patch fall back to
guessed profile count if the profiled count is of low quality.  Will
send it out in GCC10.

Thanks,
bin
>

> Mark them as ALWAYS_INLINE instead.

>

> gcc/:

>

> 2019-01-14  Andi Kleen  <ak@linux.intel.com>

>

>         * vec.h (using_auto_storage, release): Mark as ALWAYS_INLINE.

> ---

>  gcc/vec.h | 4 ++--

>  1 file changed, 2 insertions(+), 2 deletions(-)

>

> diff --git a/gcc/vec.h b/gcc/vec.h

> index 407269c5ad3..1f5b78b1fac 100644

> --- a/gcc/vec.h

> +++ b/gcc/vec.h

> @@ -1664,7 +1664,7 @@ vec<T, va_heap, vl_ptr>::create (unsigned nelems MEM_STAT_DECL)

>  /* Free the memory occupied by the embedded vector.  */

>

>  template<typename T>

> -inline void

> +ALWAYS_INLINE void

>  vec<T, va_heap, vl_ptr>::release (void)

>  {

>    if (!m_vec)

> @@ -1940,7 +1940,7 @@ vec<T, va_heap, vl_ptr>::reverse (void)

>  }

>

>  template<typename T>

> -inline bool

> +ALWAYS_INLINE bool

>  vec<T, va_heap, vl_ptr>::using_auto_storage () const

>  {

>    return m_vec->m_vecpfx.m_using_auto_storage;

> --

> 2.19.1

>

Patch

diff --git a/gcc/vec.h b/gcc/vec.h
index 407269c5ad3..1f5b78b1fac 100644
--- a/gcc/vec.h
+++ b/gcc/vec.h
@@ -1664,7 +1664,7 @@  vec<T, va_heap, vl_ptr>::create (unsigned nelems MEM_STAT_DECL)
 /* Free the memory occupied by the embedded vector.  */
 
 template<typename T>
-inline void
+ALWAYS_INLINE void
 vec<T, va_heap, vl_ptr>::release (void)
 {
   if (!m_vec)
@@ -1940,7 +1940,7 @@  vec<T, va_heap, vl_ptr>::reverse (void)
 }
 
 template<typename T>
-inline bool
+ALWAYS_INLINE bool
 vec<T, va_heap, vl_ptr>::using_auto_storage () const
 {
   return m_vec->m_vecpfx.m_using_auto_storage;