EricWF requested changes to this revision.
EricWF added a comment.
This revision now requires changes to proceed.
Almost LGTM. Just a couple of inline comments left. Thanks for working on this!
Comment at: libcxx/include/memory:3691
+ && __has_builtin(__at
hxy9243 updated this revision to Diff 84565.
hxy9243 added a comment.
Addresses comments from @EricWF.
Thanks for reviewing, I know it takes a lot of energy. It helped me learn a lot.
Repository:
rL LLVM
https://reviews.llvm.org/D24991
Files:
libcxx/include/memory
libcxx/src/memory.cpp
mclow.lists added inline comments.
Comment at: libcxx/include/memory:3700
+
+template
+inline T
`template `, please.
Otherwise when some client code does `#define T true` (yes, I've seen that!)
this breaks. `_Tp` is a reserved identifier, and if they use that
hxy9243 updated this revision to Diff 84569.
hxy9243 added a comment.
Addresses comments from @mclow.lists.
Repository:
rL LLVM
https://reviews.llvm.org/D24991
Files:
libcxx/include/memory
libcxx/src/memory.cpp
Index: libcxx/src/memory.cpp
===
EricWF accepted this revision.
EricWF added a comment.
This revision is now accepted and ready to land.
LGTM.
Repository:
rL LLVM
https://reviews.llvm.org/D24991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bi
This revision was automatically updated to reflect the committed changes.
Closed by commit rL292184: [Test patch] Inline hot functions in libcxx
shared_ptr (authored by hxy9243).
Changed prior to commit:
https://reviews.llvm.org/D24991?vs=84569&id=84622#toc
Repository:
rL LLVM
https://revie
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 hea
hxy9243 set the repository for this revision to rL LLVM.
hxy9243 updated this revision to Diff 82958.
hxy9243 added a comment.
Move the header back in its place, and only copy over necessary parts. Now call
__atomic_add_fetch from inside the functions.
Repository:
rL LLVM
https://reviews.llv
hxy9243 updated this revision to Diff 82962.
hxy9243 marked 4 inline comments as done.
hxy9243 added a comment.
Minor fix, remove redundant inlines.
Repository:
rL LLVM
https://reviews.llvm.org/D24991
Files:
libcxx/include/memory
libcxx/src/memory.cpp
Index: libcxx/src/memory.cpp
==
hxy9243 marked an inline comment as done.
hxy9243 added a comment.
Addressed previous issues in the comments. The patch still shows consistent
perf uplift in proprietary benchmark on shared_ptr.
@EricWF @sebpop @hiraditya Any thoughts?
Repository:
rL LLVM
https://reviews.llvm.org/D24991
kubabrecka added inline comments.
Comment at: libcxx/include/memory:3702
+inline T
+__libcpp_atomic_increment(T& t) _NOEXCEPT
+{
I don't think this should be named `__libcpp_atomic_increment`, because it uses
relaxed ordering and thus it's not a generic incremen
hxy9243 updated this revision to Diff 84004.
hxy9243 added a comment.
Minor changes on function names. Rename __libcpp_atomic_* to
__libcpp_atomic_refcount_*.
Repository:
rL LLVM
https://reviews.llvm.org/D24991
Files:
libcxx/include/memory
libcxx/src/memory.cpp
Index: libcxx/src/memory
hxy9243 added a comment.
Ping. @mclow.lists, @EricWF, any ideas on this patch?
Thanks very much!
https://reviews.llvm.org/D24991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
sebpop added a comment.
@mclow.lists could you please have a last look at this patch: the change is for
a performance improvement (20% uplift on a proprietary benchmark), and all the
issues mentioned in the review have been addressed.
The existing synthetic benchmark shows an overall improvement
hxy9243 added a comment.
Thanks for pointing out. It's true that it may cause ABI breakage. It would be
nice to keep compatibility while getting the performance benefits from inlining.
I've tested the patch with google-benchmark/util_smartptr_libcxx shipped with
libcxx on x86_64 server, and att
sebpop added a comment.
In https://reviews.llvm.org/D24991#565861, @EricWF wrote:
> In https://reviews.llvm.org/D24991#565715, @mclow.lists wrote:
>
> > How does this play with existing binaries? Applications that expect these
> > functions to exist in the dylib?
>
>
> This patch is majorly ABI
hiraditya added a comment.
Marshall suggests using macro as we discussed offline. For some reason the
reply does not appear here:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20161010/173780.html
Repository:
rL LLVM
https://reviews.llvm.org/D24991
_
sebpop added a comment.
In https://reviews.llvm.org/D24991#571056, @hiraditya wrote:
> Marshall suggests using macro as we discussed offline. For some reason the
> reply does not appear here:
> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20161010/173780.html
Ping.
Repository:
sebpop removed rL LLVM as the repository for this revision.
sebpop updated this revision to Diff 75908.
sebpop added a comment.
The patch also implements the idea that Marshall proposed in:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20161010/173780.html
> I have an idea; it involves
sebpop marked 2 inline comments as done.
sebpop added inline comments.
Comment at: libcxx/include/memory:3802
+{
+ return __libcpp_atomic_add(&t, 1, _AO_Relaxed);
+}
EricWF wrote:
> Why add `increment` and `decrement` at all? Just manually inline
> `__libcpp_at
hxy9243 accepted this revision.
hxy9243 added a comment.
This revision is now accepted and ready to land.
Looks good to me.
Notice that the performance gain can only be observed when compiled with the
updated C++ header files.
https://reviews.llvm.org/D24991
_
sebpop added a comment.
Ping: Eric, Marshall, could you please approve or comment on this patch?
Thanks!
https://reviews.llvm.org/D24991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi
kubabrecka added a comment.
Just a question: TSan intercepts on the dylib functions, namely
`__release_shared`, to track the atomic accesses. Can you make sure this
doesn't break? There's a few testcases for this in compiler-rt.
https://reviews.llvm.org/D24991
___
sebpop added a comment.
In https://reviews.llvm.org/D24991#583877, @kubabrecka wrote:
> Just a question: TSan intercepts on the dylib functions, namely
> `__release_shared`, to track the atomic accesses. Can you make sure this
> doesn't break? There's a few testcases for this in compiler-rt.
kubabrecka added a comment.
In https://reviews.llvm.org/D24991#586219, @sebpop wrote:
> I just ran ninja check-all with and without this patch and there are no
> regressions in compiler-rt on an x86_64-linux machine.
The TSan interceptors (and testcases) are Darwin-only at this point. I'll ru
hxy9243 added a comment.
In https://reviews.llvm.org/D24991#586248, @kubabrecka wrote:
> In https://reviews.llvm.org/D24991#586219, @sebpop wrote:
>
> > I just ran ninja check-all with and without this patch and there are no
> > regressions in compiler-rt on an x86_64-linux machine.
>
>
> The TS
kubabrecka added a comment.
This passes TSan tests on Darwin. LGTM.
https://reviews.llvm.org/D24991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
sebpop added a comment.
@mclow.lists, @EricWF, ok to commit the patch?
Thanks,
Sebastian
https://reviews.llvm.org/D24991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
hxy9243 created this revision.
hxy9243 added reviewers: sebpop, hiraditya, wmi.
hxy9243 added a subscriber: cfe-commits.
hxy9243 set the repository for this revision to rL LLVM.
This patch moves some existing functions from the memory.cpp to the memory
header file, so that they could be properly
hxy9243 updated this revision to Diff 73293.
hxy9243 added a comment.
Addresses comments from @halyavin, rename "atomic_support.h" to
"__atomic_support" to avoid collisions with application headers.
Repository:
rL LLVM
https://reviews.llvm.org/D24991
Files:
libcxx/include/__atomic_support
mclow.lists added a comment.
How does this play with existing binaries? Applications that expect these
functions to exist in the dylib?
Repository:
rL LLVM
https://reviews.llvm.org/D24991
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
EricWF added a comment.
In https://reviews.llvm.org/D24991#565715, @mclow.lists wrote:
> How does this play with existing binaries? Applications that expect these
> functions to exist in the dylib?
This patch is majorly ABI breaking, although we could probably find a
formulation that wasn't.
sebpop added a comment.
In https://reviews.llvm.org/D24991#565861, @EricWF wrote:
> Please provide benchmark tests which demonstrate that these benefits are
> concrete and not just "potential". Moving methods out of the dylib is no
> easy task so I would like hard evidence that it's worth whil
EricWF added a comment.
In https://reviews.llvm.org/D24991#566140, @sebpop wrote:
> In https://reviews.llvm.org/D24991#565861, @EricWF wrote:
>
> > Please provide benchmark tests which demonstrate that these benefits are
> > concrete and not just "potential". Moving methods out of the dylib is
On Thu, Oct 13, 2016 at 11:48 AM, Sebastian Pop wrote:
> sebpop added a comment.
>
> In https://reviews.llvm.org/D24991#565861, @EricWF wrote:
>
> > In https://reviews.llvm.org/D24991#565715, @mclow.lists wrote:
> >
> > > How does this play with existing binaries? Applications that expect
> thes
halyavin added a subscriber: halyavin.
Comment at: libcxx/include/atomic_support.h:1
@@ +1,2 @@
+//===--===
+//
Non-standard include files in the main include directory must start with __ to
36 matches
Mail list logo
