[libc++] Optimize `exception_ptr`, especially for the empty case

[fwolff]

Summary

Fixes #44892. The example given there does indeed optimize away completely with my changes.

[philnik]

I think this could be implemented in a non ABI breaking way by using __attribute__((__gnu_inline__)). See https://godbolt.org/z/W83rTb714 for an example. If you want to allow it in ABIv1 this way you should probably just disable the magic in ABIv2.

libcxx/src/support/runtime/exception_pointer_cxxabi.ipp

17

Is there a reason for this additional indirection?

[Mordante]

Can you add some benchmarks to show how much improvements these changes bring?

[avogelsgesang]

afaict, this should also fix https://github.com/llvm/llvm-project/issues/39333

[fwolff]

In D122536#3409909, @philnik wrote:

I think this could be implemented in a non ABI breaking way by using __attribute__((__gnu_inline__)). See https://godbolt.org/z/W83rTb714 for an example. If you want to allow it in ABIv1 this way you should probably just disable the magic in ABIv2.

Sorry, but I haven't been able to get this to work. I don't really understand your example, and the documentation for gnu_inline is pretty sparse.

In D122536#3443999, @avogelsgesang wrote:

afaict, this should also fix https://github.com/llvm/llvm-project/issues/39333

Yes, indeed, thanks for pointing this out!

In D122536#3414390, @Mordante wrote:

Can you add some benchmarks to show how much improvements these changes bring?

I don't know what a sensible benchmark for this would look like, but have a look at the generated assembly:

Example 1 (from #44892): https://godbolt.org/z/YGq86cszM. With my changes, this compiles to just

0000000000000000 <_Z4testv>:
   0:	b0 01                	mov    al,0x1
   2:	c3                   	ret

Example 2 (from #39333): https://godbolt.org/z/oee13ennv. With my changes, this becomes

0000000000000000 <main>:
   0:	31 c0                	xor    eax,eax
   2:	c3                   	ret

[fwolff]

libcxx/src/support/runtime/exception_pointer_cxxabi.ipp

17

There are two implementations of __incr/decr_refcount(), one here and one in exception_pointer_unimplemented.ipp. But maybe the latter one is unnecessary? I'm not sure.

[philnik]

Adding the move constructor and assignment operator isn't ABI-breaking AFAICT. IIUC the ABI breaking part of this patch is that the functions don't get generated anymore in the dylib. Using __attribute__((__gnu_inline__)) allows us to suppress generating the functions if they're not inlined, but allows the compiler to inline the function. Now if we don't apply the attribute when building the library the functions will be emitted, but otherwise they won't. This way we have the same linkage and so on for the dylib function, but have all the optimization opportunities of an inline function.

[fwolff]

In D122536#3445811, @philnik wrote:

Adding the move constructor and assignment operator isn't ABI-breaking AFAICT. IIUC the ABI breaking part of this patch is that the functions don't get generated anymore in the dylib. Using __attribute__((__gnu_inline__)) allows us to suppress generating the functions if they're not inlined, but allows the compiler to inline the function. Now if we don't apply the attribute when building the library the functions will be emitted, but otherwise they won't. This way we have the same linkage and so on for the dylib function, but have all the optimization opportunities of an inline function.

Yes, of course you're right about the move constructor and assignment operator, I had gotten this mixed up.

The GCC documentation says about gnu_inline that

The way to use this is to put a function definition in a header file with this attribute, and put another copy of the function, without extern, in a library file.

and

In C++, this attribute does not depend on extern in any way, but it still requires the inline keyword to enable its special behavior.

So I don't understand how this is supposed to work in C++. Let's consider a small example:
a.h:

#pragma once

struct A {
  int foo() { return 1; }
};

a.cc:

#include "a.h"

int A::foo() { return 1; }

Could you annotate this example with gnu_inline to illustrate your point? And, if you are going to use any #ifdefs, what should they check for in libc++? Thank you!

[philnik]

I think I'm abusing __gnu_inline__ here a bit, but hey:

First of all, you have to have both the declaration and the definition inside the header, because neither clang nor GCC are happy to have an out of line definition of an already defined function.
a.h:

#pragma once

#ifdef EMIT_FUNCTION
#  define GNU_INLINE
#else
#  define GNU_INLINE __attribute__((__gnu_inline__)) inline
#endif

struct A {
  GNU_INLINE int foo();
};

// Defining the function this way forces the compiler to emit the function when it's not marked inline,
// but if it's maked '__attribute__((__gnu_inline__)) inline' it forces the compiler to _not_ emit the function.
GNU_INLINE int A::foo() {
  return 1;
}

a.cpp:

#define EMIT_FUNCTION
#include "a.h"

You have to use a different name and suppress a warning, but I left that away for simplicity.

[fwolff]

Thanks for your help @philnik. I have now revised my implementation so that the ABI break is no longer required.

[Mordante]

In D122536#3414390, @Mordante wrote:

Can you add some benchmarks to show how much improvements these changes bring?

I don't know what a sensible benchmark for this would look like, but have a look at the generated assembly:

Example 1 (from #44892): https://godbolt.org/z/YGq86cszM. With my changes, this compiles to just

0000000000000000 <_Z4testv>:
   0:	b0 01                	mov    al,0x1
   2:	c3                   	ret

Example 2 (from #39333): https://godbolt.org/z/oee13ennv. With my changes, this becomes

0000000000000000 <main>:
   0:	31 c0                	xor    eax,eax
   2:	c3                   	ret

That assembly looks convincing, thanks.

libcxx/include/exception

144

I would like some comment describing this _LIBCPP_EXCEPTION_PTR_GNU_INLINE magic. Reading the other comments in this review make it clear, could you add short summary of that information here?

145

We should validate whether the compiler supports this attribute.

189

Same at line 167.

[fwolff]

Thanks for reviewing this @Mordante! I think I've addressed all of your comments, and I've added a suppression for -Wgnu-inline-cpp-without-extern, which will hopefully fix the failing tests.

[fwolff]

libcxx/include/exception

145

The documentation seems to imply that either __GNUC_GNU_INLINE__ or __GNUC_STDC_INLINE__ will be defined if the __gnu_inline__ attribute is supported, so that's what I am now checking for. Let me know if you had something else in mind.

[philnik]

libcxx/include/exception

150

I think you mean the __gnu_inline__ attribute here.

153

Do you mean _LIBCPP_EXCEPTION_PTR_GNU_INLINE here? If yes, please just put it here. It's a bit confusing otherwise.

154–155

__gnu_inline__ doesn't force the functions to be inlined. It disables the generation of the functions.

158

You can use __has_attribute(__gnu_inline__) to check if the compiler supports it. That also works for any other attribute.

250

I don't think that the comment is necessary here, but I also don't object.

252–254

libcxx/src/exception.cpp

9–11

Why do you guard the macro definition here? Could you add a short comment here why you are defining this?

[philnik]

Also please update the description.

[fwolff]

libcxx/src/exception.cpp

9–11

I think this was wrong; the macro definition can be unguarded and we unfortunately need more preprocessor guards in the exception header, because the glibcxx ABI doesn't have __incr/decr_refcount()...

[avogelsgesang]

Are there still any remaining issues with this change?

As an user of std::exception_ptr (mostly for async callbacks and coroutines), I would love to see this land in LLVM 15 :)

[Mordante]

In D122536#3628912, @avogelsgesang wrote:

Are there still any remaining issues with this change?

As an user of std::exception_ptr (mostly for async callbacks and coroutines), I would love to see this land in LLVM 15 :)

The latest patch didn't pass our pre-commit CI. So that needs to be fixed and it would be good to rebase the patch against main.

[fwolff]

@Mordante I think the main problem were the inconsistent abilist files, which I have attempted to fix here (unfortunately generate-cxx-abilist didn't really work for me, as it created lots of other entries for things I didn't touch, so I had to do it manually). I've also rebased the patch, though the rebase went cleanly.

[fwolff]

In D122536#3631434, @fwolff wrote:

I think the main problem were the inconsistent abilist files, which I have attempted to fix here

This problem does indeed appear to be fixed now, judging by the latest CI run. But there are now some problems on Mac OS, which I cannot reproduce because I don't have such a machine. Perhaps someone else can have a look at this?

[philnik]

In D122536#3633355, @fwolff wrote:

In D122536#3631434, @fwolff wrote:

I think the main problem were the inconsistent abilist files, which I have attempted to fix here

This problem does indeed appear to be fixed now, judging by the latest CI run. But there are now some problems on Mac OS, which I cannot reproduce because I don't have such a machine. Perhaps someone else can have a look at this?

The problem is that you have introduced new symbols to the dylib. I think the easiest way to avoid this is to use __cxa_{increment, decrement}_exception_refcount in the header directly.

[fwolff]

In D122536#3633419, @philnik wrote:

The problem is that you have introduced new symbols to the dylib. I think the easiest way to avoid this is to use __cxa_{increment, decrement}_exception_refcount in the header directly.

How is this supposed to work? Including cxxabi.h in the header breaks everything, and nowhere else in libcxx/include/ do any __cxa_* functions get called. Also, there must be a way to distinguish exception_pointer_cxxabi.ipp from exception_pointer_unimplemented.ipp in the header if we're not going to introduce any additional symbols; how should I go about doing this?

[philnik]

In D122536#3633708, @fwolff wrote:

In D122536#3633419, @philnik wrote:

The problem is that you have introduced new symbols to the dylib. I think the easiest way to avoid this is to use __cxa_{increment, decrement}_exception_refcount in the header directly.

How is this supposed to work? Including cxxabi.h in the header breaks everything, and nowhere else in libcxx/include/ do any __cxa_* functions get called. Also, there must be a way to distinguish exception_pointer_cxxabi.ipp from exception_pointer_unimplemented.ipp in the header if we're not going to introduce any additional symbols; how should I go about doing this?

I think you should only enable the optimization for known-good C++ABIs (so only libc++abi currently) and to forward-declare the __cxa_ functions. That means the default still is calling the functions in exception_pointer_unimplemented.ipp.

[fwolff]

Thanks for your help @philnik. I'm still unsure what exactly I should check for in the exception header; I went for

#if defined(_LIBCPP_ABI_VERSION) || defined(LIBCXXRT) || defined(LIBCXX_BUILDING_LIBCXXABI)

because the first two are used in libcxx/src/exception.cpp to indicate that the exception_pointer_cxxabi.ipp implementation is to be used, and it didn't work without the last one, but it doesn't really look right to me, so please comment.

[philnik]

In D122536#3634273, @fwolff wrote:

Thanks for your help @philnik. I'm still unsure what exactly I should check for in the exception header; I went for

#if defined(_LIBCPP_ABI_VERSION) || defined(LIBCXXRT) || defined(LIBCXX_BUILDING_LIBCXXABI)

because the first two are used in libcxx/src/exception.cpp to indicate that the exception_pointer_cxxabi.ipp implementation is to be used, and it didn't work without the last one, but it doesn't really look right to me, so please comment.

I'm not actually sure what the right is here. I think @ldionne should take a look, as he is more familiar with these things.

libcxx/include/exception

 
 class _LIBCPP_TYPE_VIS exception_ptr;
 
 _LIBCPP_FUNC_VIS exception_ptr current_exception() _NOEXCEPT;
 _LIBCPP_NORETURN _LIBCPP_FUNC_VIS void rethrow_exception(exception_ptr);
 
 #ifndef _LIBCPP_ABI_MICROSOFT
 
+// Previously, the copy constructor, assignment operator, and destructor of
+// exception_ptr were defined out-of-line, which prevented useful compiler
+// optimizations (especially for the case where the exception_ptr is known to
+// be empty). Changing the out-of-line definitions to inline definitions is an
+// ABI break, however; to prevent this, we have to make sure the symbols
+// remain available in the libc++ library, in addition to being defined inline
+// here in this header. To this end, we use the __gnu_inline__ attribute via the
+// _LIBCPP_EXCEPTION_PTR_GNU_INLINE macro: The macro is defined as empty in
+// src/exception.cpp, forcing the definitions of the aforementioned functions to
+// be emitted and included in the library. When users of libc++ (re)compile their
+// code, however, the _LIBCPP_EXCEPTION_PTR_GNU_INLINE macro won't be defined,
+// and thus the __gnu_inline__ attribute will suppress generation of these
+// functions while making their definitions available for inlining.
+#ifndef _LIBCPP_EXCEPTION_PTR_GNU_INLINE
+#if !__has_attribute(__gnu_inline__)
+#error "this compiler does not appear to support the __gnu_inline__ attribute!"
+#endif
+
+#ifndef __GLIBCXX__
+#define _LIBCPP_EXCEPTION_PTR_GNU_INLINE __attribute__((__gnu_inline__)) inline
+#else
+#define _LIBCPP_EXCEPTION_PTR_GNU_INLINE /*empty*/
+#endif
+#endif
+
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wgnu-inline-cpp-without-extern")
+
 class _LIBCPP_TYPE_VIS exception_ptr
 {
     void* __ptr_;
+
+#ifndef __GLIBCXX__
+    void __incr_refcount() _NOEXCEPT;
+    void __decr_refcount() _NOEXCEPT;
+#endif
+
 public:
     _LIBCPP_INLINE_VISIBILITY exception_ptr() _NOEXCEPT : __ptr_() {}
     _LIBCPP_INLINE_VISIBILITY exception_ptr(nullptr_t) _NOEXCEPT : __ptr_() {}
 
-    exception_ptr(const exception_ptr&) _NOEXCEPT;
-    exception_ptr& operator=(const exception_ptr&) _NOEXCEPT;
-    ~exception_ptr() _NOEXCEPT;
+    _LIBCPP_EXCEPTION_PTR_GNU_INLINE exception_ptr(const exception_ptr&) _NOEXCEPT;
+    _LIBCPP_EXCEPTION_PTR_GNU_INLINE exception_ptr& operator=(const exception_ptr&) _NOEXCEPT;
+    _LIBCPP_EXCEPTION_PTR_GNU_INLINE ~exception_ptr() _NOEXCEPT;
+
+    _LIBCPP_INLINE_VISIBILITY exception_ptr(exception_ptr&& __rhs) _NOEXCEPT : __ptr_(__rhs.__ptr_) {
+        __rhs.__ptr_ = nullptr;
+    }
+
+    // Move assignment is not available for the glibcxx ABI because it
+    // doesn't have __decr_refcount().
+#ifndef __GLIBCXX__
+    _LIBCPP_INLINE_VISIBILITY exception_ptr& operator=(exception_ptr&& __rhs) _NOEXCEPT {
+        if (__ptr_ != nullptr)
+            __decr_refcount();
+        __ptr_ = __rhs.__ptr_;
+        __rhs.__ptr_ = nullptr;
+        return *this;
+    }
+#endif
+
+    _LIBCPP_INLINE_VISIBILITY void swap(exception_ptr& __rhs) _NOEXCEPT {
+        void *__tmp = __ptr_;
+        __ptr_ = __rhs.__ptr_;
+        __rhs.__ptr_ = __tmp;
+    }
 
     _LIBCPP_INLINE_VISIBILITY explicit operator bool() const _NOEXCEPT
     {return __ptr_ != nullptr;}
 
     friend _LIBCPP_INLINE_VISIBILITY
     bool operator==(const exception_ptr& __x, const exception_ptr& __y) _NOEXCEPT
         {return __x.__ptr_ == __y.__ptr_;}
 
     friend _LIBCPP_INLINE_VISIBILITY
     bool operator!=(const exception_ptr& __x, const exception_ptr& __y) _NOEXCEPT
         {return !(__x == __y);}
 
     friend _LIBCPP_FUNC_VIS exception_ptr current_exception() _NOEXCEPT;
     friend _LIBCPP_FUNC_VIS void rethrow_exception(exception_ptr);
 };
 
+// If __GLIBCXX__ is defined, the exception_ptr from libsupc++ is used (which
+// works because it happens to have the same layout as our exception_ptr).
+// The constructor, assignment operator, and destructor should just delegate
+// to the libsupc++ implementations then, so we don't provide definitions for
+// them here.
+#ifndef __GLIBCXX__
+_LIBCPP_EXCEPTION_PTR_GNU_INLINE
+exception_ptr::exception_ptr(const exception_ptr& __rhs) _NOEXCEPT : __ptr_(__rhs.__ptr_) {
+    if (__rhs.__ptr_ != nullptr)
+        __incr_refcount();
+}
+
+_LIBCPP_EXCEPTION_PTR_GNU_INLINE
+exception_ptr& exception_ptr::operator=(const exception_ptr& __rhs) _NOEXCEPT {
+    if (__ptr_ != __rhs.__ptr_) {
+        if (__ptr_ != nullptr)
+            __decr_refcount();
+        __ptr_ = __rhs.__ptr_;
+        if (__ptr_ != nullptr)
+            __incr_refcount();
+    }
+    return *this;
+}
+
+_LIBCPP_EXCEPTION_PTR_GNU_INLINE
+exception_ptr::~exception_ptr() _NOEXCEPT {
+    if (__ptr_ != nullptr)
+        __decr_refcount();
+}
+#endif // __GLIBCXX__
+
+_LIBCPP_DIAGNOSTIC_POP // pops CLANG_DIAGNOSTIC_IGNORED("-Wgnu-inline-cpp-without-extern")
+
+inline _LIBCPP_INLINE_VISIBILITY void
+swap(exception_ptr& __lhs, exception_ptr& __rhs) _NOEXCEPT {
+    __lhs.swap(__rhs);
+}
+
 template<class _Ep>
 _LIBCPP_INLINE_VISIBILITY exception_ptr
 make_exception_ptr(_Ep __e) _NOEXCEPT
 {
 #ifndef _LIBCPP_NO_EXCEPTIONS
     try
     {
         throw __e;

libcxx/src/exception.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
+#define _LIBCPP_EXCEPTION_PTR_GNU_INLINE /*empty*/
 #include <exception>
+
 #include <new>
 #include <typeinfo>
 
 #if defined(LIBCXXRT) || defined(LIBCXX_BUILDING_LIBCXXABI)
   #include <cxxabi.h>
   using namespace __cxxabiv1;
   #define HAVE_DEPENDENT_EH_ABI 1
 #endif

libcxx/src/support/runtime/exception_pointer_cxxabi.ipp

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef HAVE_DEPENDENT_EH_ABI
 #error this header may only be used with libc++abi or libcxxrt
 #endif
 
 namespace std {
 
-exception_ptr::~exception_ptr() noexcept {
+void exception_ptr::__decr_refcount() noexcept {
   __cxa_decrement_exception_refcount(__ptr_);
 }
 
-exception_ptr::exception_ptr(const exception_ptr& other) noexcept
-    : __ptr_(other.__ptr_)
+void exception_ptr::__incr_refcount() noexcept
 {
     __cxa_increment_exception_refcount(__ptr_);
 }
 
-exception_ptr& exception_ptr::operator=(const exception_ptr& other) noexcept
-{
-    if (__ptr_ != other.__ptr_)
-    {
-        __cxa_increment_exception_refcount(other.__ptr_);
-        __cxa_decrement_exception_refcount(__ptr_);
-        __ptr_ = other.__ptr_;
-    }
-    return *this;
-}
-
 nested_exception::nested_exception() noexcept
     : __ptr_(current_exception())
 {
 }
 
 nested_exception::~nested_exception() noexcept
 {
 }

libcxx/src/support/runtime/exception_pointer_unimplemented.ipp

 // -*- C++ -*-
 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include <stdio.h>
 #include <stdlib.h>
 
 namespace std {
 
-exception_ptr::~exception_ptr() noexcept
+void exception_ptr::__decr_refcount() noexcept
 {
 #  warning exception_ptr not yet implemented
   fprintf(stderr, "exception_ptr not yet implemented\n");
   ::abort();
 }
 
-exception_ptr::exception_ptr(const exception_ptr& other) noexcept
-    : __ptr_(other.__ptr_)
-{
-#  warning exception_ptr not yet implemented
-  fprintf(stderr, "exception_ptr not yet implemented\n");
-  ::abort();
-}
-
-exception_ptr& exception_ptr::operator=(const exception_ptr& other) noexcept
+void exception_ptr::__incr_refcount() noexcept
 {
 #  warning exception_ptr not yet implemented
   fprintf(stderr, "exception_ptr not yet implemented\n");
   ::abort();
 }
 
 nested_exception::nested_exception() noexcept
     : __ptr_(current_exception())