[PATCH] D24991: Inline hot functions in libcxx shared_ptr implementation.

Eric Fiselier via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Dec 30 02:47:44 PST 2016


EricWF requested changes to this revision.
EricWF added a reviewer: EricWF.
EricWF added a comment.
This revision now requires changes to proceed.

Added a bunch of inline comments.

The biggest requested change is removing the `__atomic_support` header. We only need one atomic call within the headers. It's overkill to add a new header.



================
Comment at: libcxx/include/__atomic_support:1
+//===----------------------------------------------------------------------===////
+//
----------------
I would greatly prefer if this patch didn't add another header, and simply defined `__libcpp_atomic_increment` and `__libcpp_atomic_decrement` in place of `__atomic_inc_dec::increment`/`__atomic_inc_dec::decrement`.


================
Comment at: libcxx/include/memory:3802
+{
+  return __libcpp_atomic_add(&t, 1, _AO_Relaxed);
+}
----------------
sebpop wrote:
> EricWF wrote:
> > Why add `increment` and `decrement` at all? Just manually inline `__libcpp_atomic_add` at the callsites.
> I like the idea to manually inline the inc and dec functions.
> What should we do with the NOTE: above?
> 
> // NOTE: Relaxed and acq/rel atomics (for increment and decrement respectively)
> // should be sufficient for thread safety.
> // See https://llvm.org/bugs/show_bug.cgi?id=22803
> 
> should we just go ahead and remove the note, or you want to have it where inc/dec are called?  (about a dozen places.) 
Neremind about the manually inlining bit.

Please remove the `__atomic_inc_dec` namespace and rename `increment` to `__libcpp_atomic_increment` and `decrement` to `__libcpp_atomic_decrement`.

Please also remove the `__atomic_support` header and instead simply call `__atomic_add_fetch` from inside the functions.


================
Comment at: libcxx/include/memory:3911
+#ifdef _LIBCPP_BUILDING_MEMORY
     void __add_shared() _NOEXCEPT;
     bool __release_shared() _NOEXCEPT;
----------------
Please apply `_LIBCPP_FUNC_VIS` to both of these methods.


================
Comment at: libcxx/include/memory:3914
+#else
+    inline _LIBCPP_INLINE_VISIBILITY
+    void __add_shared() _NOEXCEPT {
----------------
the `inline` in redundant if you define the function inside the class.


================
Comment at: libcxx/include/memory:3948
+#ifdef _LIBCPP_BUILDING_MEMORY
     void __add_shared() _NOEXCEPT;
     void __add_weak() _NOEXCEPT;
----------------
Please add `_LIBCPP_FUNC_VIS` to the three methods.


================
Comment at: libcxx/include/memory:3952
+#else
+    inline _LIBCPP_INLINE_VISIBILITY
+    void __add_shared() _NOEXCEPT {
----------------
`inline` is redundant here.


https://reviews.llvm.org/D24991





More information about the cfe-commits mailing list