sebpop added a comment.
In https://reviews.llvm.org/D22782#495436, @mclow.lists wrote:
> Do we have a test for the problem that this is solving?
I think we can write a testcase that shows that copy constructors are not
optimized away unless the string constructor is inlined.
This patch fixes
Author: spop
Date: Thu Aug 11 11:51:48 2016
New Revision: 278356
URL: http://llvm.org/viewvc/llvm-project?rev=278356=rev
Log:
Add 'inline' attribute to __init to inline the basic_string's constructor
basic_string's constructor calls init which was not getting inlined. This
prevented
sebpop added a comment.
In https://reviews.llvm.org/D22782#512337, @EricWF wrote:
> I would love to see a benchmark with this, but I've done enough investigating
> on my own that I *know* this patch is beneficial.
This patch was motivated by perf analysis we did on a proprietary benchmark in
This revision was automatically updated to reflect the committed changes.
Closed by commit rL278356: Add 'inline' attribute to __init to inline the
basic_string's constructor (authored by spop).
Changed prior to commit:
https://reviews.llvm.org/D22782?vs=67597=67699#toc
Repository:
rL LLVM
sebpop added a comment.
I like the patch.
Thanks for removing #ifdefs from the code: it improves readability in general.
Would it be possible to move the __throw_* functions in a same .h file to avoid
having them all over?
Comment at: include/array:212
@@ -214,3 +211,3 @@
sebpop accepted this revision.
sebpop added a comment.
This revision is now accepted and ready to land.
Very nice cleanup.
Maybe you can move some more #ifdefs into __throw_* functions, although as is
LGTM.
Thanks!
https://reviews.llvm.org/D23855
sebpop abandoned this revision.
sebpop added a comment.
Agreed, this is a test for llvm/opt.
I will submit another patch.
https://reviews.llvm.org/D24097
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
sebpop created this revision.
sebpop added reviewers: jmolloy, danielcdh, davidxl, mkuper, dberlin.
sebpop added subscribers: llvm-commits, cfe-commits.
Add testcases to clang test/ to make sure that the combined middle-end
optimizations do not regress on optimizing the number of loads in the
sebpop added inline comments.
Comment at: clang/lib/CodeGen/CGCXX.cpp:137
@@ -136,1 +136,3 @@
+ // r254170: Disallow aliases to available_externally.
+ if (TargetLinkage == llvm::GlobalValue::AvailableExternallyLinkage)
Please remove the reference to r254170.
sebpop added a comment.
The change looks good to me. Somebody else should approve.
https://reviews.llvm.org/D24682
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL282259: set the underlying value of “#pragma STDC
FP_CONTRACT” on by default (authored by spop).
Changed prior to commit:
https://reviews.llvm.org/D24481?vs=72186=72299#toc
Repository:
rL LLVM
sebpop added inline comments.
Comment at: clang/lib/CodeGen/CGCXX.cpp:140-142
@@ +139,5 @@
+ // FIXME: An extern template instantiation will create functions with
+ // linkage "AvailableExternally". In libc++, some classes also define
+ // members with attribute "AlwaysInline"
sebpop marked 2 inline comments as done.
sebpop added inline comments.
Comment at: libcxx/include/memory:3802
+{
+ return __libcpp_atomic_add(, 1, _AO_Relaxed);
+}
EricWF wrote:
> Why add `increment` and `decrement` at all? Just manually inline
>
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 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
sebpop accepted this revision.
sebpop added a reviewer: sebpop.
sebpop added a comment.
This revision is now accepted and ready to land.
This got approved in the past review.
Let's commit it now that the clang bug was fixed.
Thanks Aditya for keeping track of this.
sebpop added a comment.
If I remember correctly, we pushed the fix after 3.9 was released.
Could you please explain the problem of requiring trunk LLVM to build trunk
libcxx?
https://reviews.llvm.org/D25624
___
cfe-commits mailing list
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 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
sebpop added inline comments.
Comment at: libcxx/include/string:1841
template
+inline _LIBCPP_HEADER_INLINE_VISIBILITY
basic_string<_CharT, _Traits, _Allocator>::~basic_string()
and let's also use the define just here:
#ifndef _LIBCPP_BUILDING_STRING
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
sebpop added a comment.
In https://reviews.llvm.org/D25624#582731, @mehdi_amini wrote:
> I don't understand:
>
> 1. The motivation for this change
This is a change for performance: we have seen some benchmarks where inlining
the string dtor brings performance up by 5%: from what I remember,
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
Author: spop
Date: Thu Oct 13 19:07:57 2016
New Revision: 284179
URL: http://llvm.org/viewvc/llvm-project?rev=284179=rev
Log:
remove warnings from google-benchmarks in libcxx
Differential Revision: https://reviews.llvm.org/D25522
Patch written by Aditya Kumar.
Modified:
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
Author: spop
Date: Fri Dec 30 12:01:36 2016
New Revision: 290761
URL: http://llvm.org/viewvc/llvm-project?rev=290761=rev
Log:
improve performance of string::find
string::find used to call the generic algorithm ::find. The patch special
case string::find such that it ultimately gets converted to
26 matches
Mail list logo
