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&id=72299#toc
Repository:
rL LLVM
ht
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 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
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
Author: spop
Date: Thu Oct 13 19:07:57 2016
New Revision: 284179
URL: http://llvm.org/viewvc/llvm-project?rev=284179&view=rev
Log:
remove warnings from google-benchmarks in libcxx
Differential Revision: https://reviews.llvm.org/D25522
Patch written by Aditya Kumar.
Modified:
libcxx/trunk/be
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.
https://reviews.llvm.org/D2562
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
cfe-commits@
Author: spop
Date: Fri Dec 30 12:01:36 2016
New Revision: 290761
URL: http://llvm.org/viewvc/llvm-project?rev=290761&view=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 converte
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 @@
#els
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 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 loop
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
http://lists.llvm.org/cgi-bin/mailman/l
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.
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
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
inli
sebpop added a comment.
Other than removing that comment the patch looks good.
Thanks!
Comment at: libcxx/src/string.cpp:10
+// For keeping the definition of ~basic_string in this translation unit.
+#define _LIBCPP_BUILDING_STRING
Let's not add this comment h
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, th
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
sebpop added a comment.
In https://reviews.llvm.org/D25624#583163, @mehdi_amini wrote:
> I'd like to understand why only the destructor?
We have committed a patch to inline the constructor of std::string.
Do you think we should inline some other functions?
In https://reviews.llvm.org/D25624#58
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.
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 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
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&id=67699#toc
Repository:
rL LLV
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
Author: spop
Date: Thu Aug 11 11:51:48 2016
New Revision: 278356
URL: http://llvm.org/viewvc/llvm-project?rev=278356&view=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 optimiz
sebpop added a comment.
LGTM.
http://reviews.llvm.org/D21907
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
@@ -442,6 +442,10 @@ def warn_drv_deprecated_arg : Warning<
def warn_drv_deprecated_arg_no_relaxed_template_template_args : Warning<
"argument '-fno-relaxed-template-template-args' is deprecated">,
InGroup;
+def warn_drv_deprecated_arg_ofast : Warning<
+ "argument '-Ofast'
30 matches
Mail list logo
