Reapply "[libc++] Mark __{emplace,push}_back_slow_path as noinline (#94379)"

[philnik777]

This reverts commit 7f2e9b1.

[ldionne]

Let's say we had this bug fixed and the code went back to looking like:

vector<_Tp, _Allocator>::emplace_back(_Args&&... __args) {
    pointer __end = this->__end_;
    if (__end < this->__cap_) [[likely]] {
        __emplace_back_assume_capacity(std::forward<_Args>(__args)...);
        ++__end;
    } else {
        __end = __emplace_back_slow_path(std::forward<_Args>(__args)...);
    }
    this->__end_ = __end;

    ...
}

Then we could evolve this into something like:

vector<_Tp, _Allocator>::emplace_back(_Args&&... __args) {
    pointer __end = this->__end_;
    if (__end < this->__cap_) _LIBCPP_LIKELY_UNLESS_COMPILER_KNOWS_BETTER {
        __emplace_back_assume_capacity(std::forward<_Args>(__args)...);
        ++__end;
    } else {
        __end = __emplace_back_slow_path(std::forward<_Args>(__args)...);
    }
    this->__end_ = __end;

    ...
}

_LIBCPP_LIKELY_UNLESS_COMPILER_KNOWS_BETTER would basically be a likelihood annotation that is a no-op when the compiler knows better, such as when we're being compiled with PGO. That would probably resolve @nico 's comments from #94379 (comment)

Actually, this comment applies even if we keep our ugly workaround for the bug. The likely annotation can still be conditionalized on whether e.g. PGO is enabled. All that we'd need is a way to tell whether PGO is enabled.

[ldionne]

I think we need to tackle this comment. If this patch makes us do worse when PGO is enabled, that's something we need to fix.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Nikolas Klauser (philnik777)

Changes

This reverts commit 7f2e9b1.

---

Full diff: https://github.com/llvm/llvm-project/pull/158606.diff

1 Files Affected:

• (modified) libcxx/include/__vector/vector.h (+26-6)

diff --git a/libcxx/include/__vector/vector.h b/libcxx/include/__vector/vector.h
index 5e6572b1a82c4..27e681aeef22a 100644
--- a/libcxx/include/__vector/vector.h
+++ b/libcxx/include/__vector/vector.h
@@ -1161,6 +1161,24 @@ vector<_Tp, _Allocator>::__emplace_back_slow_path(_Args&&... __args) {
   return this->__end_;
 }
 
+// This makes the compiler inline `__else()` if `__cond` is known to be false. Currently LLVM doesn't do that without
+// the `__builtin_constant_p`, since it considers `__else` unlikely even through it's known to be run.
+// See https://llvm.org/PR154292
+template <class _If, class _Else>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 void __if_likely_else(bool __cond, _If __if, _Else __else) {
+  if (__builtin_constant_p(__cond)) {
+    if (__cond)
+      __if();
+    else
+      __else();
+  } else {
+    if (__cond) [[__likely__]]
+      __if();
+    else
+      __else();
+  }
+}
+
 template <class _Tp, class _Allocator>
 template <class... _Args>
 _LIBCPP_CONSTEXPR_SINCE_CXX20 inline
@@ -1171,12 +1189,14 @@ _LIBCPP_CONSTEXPR_SINCE_CXX20 inline
 #endif
     vector<_Tp, _Allocator>::emplace_back(_Args&&... __args) {
   pointer __end = this->__end_;
-  if (__end < this->__cap_) {
-    __emplace_back_assume_capacity(std::forward<_Args>(__args)...);
-    ++__end;
-  } else {
-    __end = __emplace_back_slow_path(std::forward<_Args>(__args)...);
-  }
+  std::__if_likely_else(
+      __end < this->__cap_,
+      [&] {
+        __emplace_back_assume_capacity(std::forward<_Args>(__args)...);
+        ++__end;
+      },
+      [&] { __end = __emplace_back_slow_path(std::forward<_Args>(__args)...); });
+
   this->__end_ = __end;
 #if _LIBCPP_STD_VER >= 17
   return *(__end - 1);

[Michael137]

Re. the LLDB failure, I suspect my fix for #154962 (which bails out of type completion when we encounter a lambda), is causing the type to be incomplete, and causing the test to fail. Will have a think about how to fix this. Might need to XFAIL the test until we fix the underlying issue properly

[Michael137]

Skipped LLDB tests in f5022bd

CI should be unblocked after rebase