[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

21

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

21

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

143

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?

144

We should validate whether the compiler supports this attribute.

163

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

144

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

149

I think you mean the __gnu_inline__ attribute here.

152

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

153–154

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

157

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

239

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

241–243

libcxx/src/exception.cpp

9–11 ↗

(On Diff #423929)

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 ↗

(On Diff #423929)

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/__config

 // std::random_device, one needs to retain the same object layout to remain ABI
 // compatible. This switch removes these workarounds for platforms that don't care
 // about ABI compatibility.
 #  define _LIBCPP_ABI_NO_RANDOM_DEVICE_COMPATIBILITY_LAYOUT
 // Remove basic_string common base
 #  define _LIBCPP_ABI_DO_NOT_EXPORT_BASIC_STRING_COMMON
 // Remove vector base class
 #  define _LIBCPP_ABI_DO_NOT_EXPORT_VECTOR_BASE_COMMON
+#  if !defined(__GLIBCXX__)
+// Add a move constructor and move assignment operator, and inline parts of the
+// definitions of the constructors, assignment operators, and destructor to
+// enable optimizing away everything if the exception_ptr is empty.
+#    define _LIBCPP_ABI_OPTIMIZED_EXCEPTION_PTR
+#  endif
 #elif _LIBCPP_ABI_VERSION == 1
 #  if !defined(_LIBCPP_OBJECT_FORMAT_COFF)
 // Enable compiling copies of now inline methods into the dylib to support
 // applications compiled against older libraries. This is unnecessary with
 // COFF dllexport semantics, since dllexport forces a non-inline definition
 // of inline functions to be emitted anyway. Our own non-inline copy would
 // conflict with the dllexport-emitted copy, so we disable it.
 #    define _LIBCPP_DEPRECATED_ABI_LEGACY_LIBRARY_DEFINITIONS_FOR_INLINE_FUNCTIONS

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
 
 class _LIBCPP_TYPE_VIS exception_ptr
 {
     void* __ptr_;
+
+#ifdef _LIBCPP_ABI_OPTIMIZED_EXCEPTION_PTR
+    void __incr_refcount() _NOEXCEPT;
+    void __decr_refcount() _NOEXCEPT;
+#endif // _LIBCPP_ABI_OPTIMIZED_EXCEPTION_PTR
+
 public:
     _LIBCPP_INLINE_VISIBILITY exception_ptr() _NOEXCEPT : __ptr_() {}
     _LIBCPP_INLINE_VISIBILITY exception_ptr(nullptr_t) _NOEXCEPT : __ptr_() {}
 
+#ifndef _LIBCPP_ABI_OPTIMIZED_EXCEPTION_PTR
     exception_ptr(const exception_ptr&) _NOEXCEPT;
     exception_ptr& operator=(const exception_ptr&) _NOEXCEPT;
     ~exception_ptr() _NOEXCEPT;
+#else
+    exception_ptr(const exception_ptr& __rhs) _NOEXCEPT : __ptr_(__rhs.__ptr_) {
+        if (__rhs.__ptr_ != nullptr)
+            __incr_refcount();
+    }
+    exception_ptr(exception_ptr&& __rhs) _NOEXCEPT : __ptr_(__rhs.__ptr_) {
+        __rhs.__ptr_ = nullptr;
+    }
+
+    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;
+    }
+    exception_ptr& operator=(exception_ptr&& __rhs) _NOEXCEPT {
+        if (__ptr_ != nullptr)
+            __decr_refcount();
+        __ptr_ = __rhs.__ptr_;
+        __rhs.__ptr_ = nullptr;
+        return *this;
+    }
+
+    ~exception_ptr() _NOEXCEPT {
+        if (__ptr_ != nullptr)
+            __decr_refcount();
+    }
+
+    _LIBCPP_INLINE_VISIBILITY
+    void swap(exception_ptr& __rhs) _NOEXCEPT {
+        void *__tmp = __ptr_;
+        __ptr_ = __rhs.__ptr_;
+        __rhs.__ptr_ = __tmp;
+    }
+#endif // _LIBCPP_ABI_OPTIMIZED_EXCEPTION_PTR
 
     _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);
 };
 
+#ifdef _LIBCPP_ABI_OPTIMIZED_EXCEPTION_PTR
+inline _LIBCPP_INLINE_VISIBILITY void
+swap(exception_ptr& __lhs, exception_ptr& __rhs) _NOEXCEPT
+{ __lhs.swap(__rhs); }
+#endif // _LIBCPP_ABI_OPTIMIZED_EXCEPTION_PTR
+
 template<class _Ep>
 _LIBCPP_INLINE_VISIBILITY exception_ptr
 make_exception_ptr(_Ep __e) _NOEXCEPT
 {
 #ifndef _LIBCPP_NO_EXCEPTIONS
     try
     {
         throw __e;
     }
     catch (...)
     {
         return current_exception();
     }
 #else
     ((void)__e);
     _VSTD::abort();
 #endif
 }
 
 #else // _LIBCPP_ABI_MICROSOFT
 
 class _LIBCPP_TYPE_VIS exception_ptr
 {
 _LIBCPP_DIAGNOSTIC_PUSH
 _LIBCPP_CLANG_DIAGNOSTIC_IGNORED("-Wunused-private-field")
     void* __ptr1_;
     void* __ptr2_;
 _LIBCPP_DIAGNOSTIC_POP
 public:
     exception_ptr() _NOEXCEPT;
     exception_ptr(nullptr_t) _NOEXCEPT;

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 {
 
+#ifndef _LIBCPP_ABI_OPTIMIZED_EXCEPTION_PTR
 exception_ptr::~exception_ptr() noexcept {
+#else
+void exception_ptr::__decr_refcount() noexcept {
+#endif // _LIBCPP_ABI_OPTIMIZED_EXCEPTION_PTR
   __cxa_decrement_exception_refcount(__ptr_);
 }
 
+#ifndef _LIBCPP_ABI_OPTIMIZED_EXCEPTION_PTR
 exception_ptr::exception_ptr(const exception_ptr& other) noexcept
     : __ptr_(other.__ptr_)
+#else
+void exception_ptr::__incr_refcount() noexcept
+#endif // _LIBCPP_ABI_OPTIMIZED_EXCEPTION_PTR
 {
     __cxa_increment_exception_refcount(__ptr_);
 }
 
+#ifndef _LIBCPP_ABI_OPTIMIZED_EXCEPTION_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;
 }
+#endif // _LIBCPP_ABI_OPTIMIZED_EXCEPTION_PTR
 
 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 {
 
+#ifndef _LIBCPP_ABI_OPTIMIZED_EXCEPTION_PTR
 exception_ptr::~exception_ptr() noexcept
+#else
+void exception_ptr::__decr_refcount() noexcept
+#endif // _LIBCPP_ABI_OPTIMIZED_EXCEPTION_PTR
 {
 #  warning exception_ptr not yet implemented
   fprintf(stderr, "exception_ptr not yet implemented\n");
   ::abort();
 }
 
+#ifndef _LIBCPP_ABI_OPTIMIZED_EXCEPTION_PTR
 exception_ptr::exception_ptr(const exception_ptr& other) noexcept
     : __ptr_(other.__ptr_)
+#else
+void exception_ptr::__incr_refcount() noexcept
+#endif // _LIBCPP_ABI_OPTIMIZED_EXCEPTION_PTR
 {
 #  warning exception_ptr not yet implemented
   fprintf(stderr, "exception_ptr not yet implemented\n");
   ::abort();
 }
 
+#ifndef _LIBCPP_ABI_OPTIMIZED_EXCEPTION_PTR
 exception_ptr& exception_ptr::operator=(const exception_ptr& other) noexcept
 {
 #  warning exception_ptr not yet implemented
   fprintf(stderr, "exception_ptr not yet implemented\n");
   ::abort();
 }
+#endif // _LIBCPP_ABI_OPTIMIZED_EXCEPTION_PTR
 
 nested_exception::nested_exception() noexcept
     : __ptr_(current_exception())
 {
 }
 
 #if !defined(__GLIBCXX__)