hiraditya abandoned this revision.
hiraditya added a comment.
In https://reviews.llvm.org/D25624#588748, @mehdi_amini wrote:
> I think @EricWF already committed the patch with just the inline keyword?
Yes he's done already. Closing this.
Thanks for your feedback.
https://reviews.llvm.org/D256
mehdi_amini added a comment.
I think @EricWF already committed the patch with just the inline keyword?
https://reviews.llvm.org/D25624
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
hiraditya added a comment.
In https://reviews.llvm.org/D25624#582756, @mehdi_amini wrote:
> In https://reviews.llvm.org/D25624#582752, @mehdi_amini wrote:
>
> > (The commit message is confusing, it mentions "Currently basic_string's
> > destructor is not getting inlined. So adding 'inline' attri
mehdi_amini added a comment.
In https://reviews.llvm.org/D25624#583883, @sebpop wrote:
> 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 w
mehdi_amini added a comment.
In https://reviews.llvm.org/D25624#584095, @hiraditya wrote:
> In https://reviews.llvm.org/D25624#583167, @mehdi_amini wrote:
>
> > I talked with Eric on IRC, he mentioned some benchmarks were ran. I'd like
> > to understand what was the baseline?
> > Here we add *b
hiraditya added a comment.
In https://reviews.llvm.org/D25624#583167, @mehdi_amini wrote:
> I talked with Eric on IRC, he mentioned some benchmarks were ran. I'd like to
> understand what was the baseline?
> Here we add *both* the inline keyword and the always_inline attribute. I'd
> like to k
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
mehdi_amini added a comment.
I talked with Eric on IRC, he mentioned some benchmarks were ran. I'd like to
understand what was the baseline?
Here we add *both* the inline keyword and the always_inline attribute. I'd like
to know if there is a benchmarks that shows that always_inline is beneficia
mehdi_amini added a comment.
Before this gets re-committed, I'd like to understand why only the destructor?
https://reviews.llvm.org/D25624
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-co
mehdi_amini added a comment.
I believe the issue is _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY itself. I
asked here:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20161024/175454.html
https://reviews.llvm.org/D25624
___
cfe-commits maili
mehdi_amini added a comment.
I see a decl:
declare hidden void
@_ZNSt3__112basic_stringIcNS_11char_traitsIcEENS_9allocatorIcEEED1Ev(%"class.std::__1::basic_string"*)
unnamed_addr #9 align 2
(Note the hidden which prevent from finding it in the dyib)
And a use (after inlining):
%1024 = ca
mehdi_amini added a comment.
FYI reproduced locally, and investigating now.
https://reviews.llvm.org/D25624
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
mehdi_amini added a comment.
In https://reviews.llvm.org/D25624#582752, @mehdi_amini wrote:
> (The commit message is confusing, it mentions "Currently basic_string's
> destructor is not getting inlined. So adding 'inline' attribute to
> ~basic_string()", please add the quote of the standard and
mehdi_amini added a comment.
Trying to figure out: it seems you are trying to address what I reported here:
https://llvm.org/bugs/show_bug.cgi?id=26498 ; right?
> Except for inline functions, declarations with types deduced
> from their initializer or return value (7.1.6.4), const
> variables
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
mehdi_amini added a comment.
I don't understand:
1. The motivation for this change
2. Why is this correct?
Can we restart the discussion here (I'm reverting to fix the LTO build failure
here:
http://lab.llvm.org:8080/green/job/clang-stage2-configure-Rlto_build/10737/console
)
https://review
hiraditya added inline comments.
Comment at: libcxx/include/string:1837
template
+inline _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY
basic_string<_CharT, _Traits, _Allocator>::~basic_string()
EricWF wrote:
> The attribute should appear on the first declaration n
EricWF accepted this revision.
EricWF added inline comments.
Comment at: libcxx/include/string:1837
template
+inline _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY
basic_string<_CharT, _Traits, _Allocator>::~basic_string()
The attribute should appear on the first d
hiraditya updated this revision to Diff 76240.
https://reviews.llvm.org/D25624
Files:
libcxx/include/string
Index: libcxx/include/string
===
--- libcxx/include/string
+++ libcxx/include/string
@@ -1834,6 +1834,7 @@
#endif // _L
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
EricWF added a comment.
Actually we don't want to copy `memory.cpp` in this case. Please use
`_LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY`. There are only examples in
`` you can follow. Make sure to put it on the in-class declaration.
https://reviews.llvm.org/D25624
__
EricWF added a comment.
Additionally _LIBCPP_EXTERN_TEMPLATE_INLINE_VISIBILITY is documented here
http://libcxx.llvm.org/docs/DesignDocs/VisibilityMacros.html.
https://reviews.llvm.org/D25624
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
hiraditya updated this revision to Diff 76192.
hiraditya added a comment.
Addressed Sebastian's comments.
https://reviews.llvm.org/D25624
Files:
libcxx/include/string
libcxx/src/string.cpp
Index: libcxx/src/string.cpp
===
---
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
hiraditya updated this revision to Diff 75915.
hiraditya added a comment.
Added macro to keep the definition of destructor of basic_string in string.cpp
https://reviews.llvm.org/D25624
Files:
libcxx/include/string
libcxx/src/string.cpp
Index: libcxx/src/string.cpp
mclow.lists added a comment.
This is the same problem as is facing https://reviews.llvm.org/D24991
https://reviews.llvm.org/D25624
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
mclow.lists added a comment.
I don't have a problem with this being marked as inline, as long as it doesn't
disappear out of the dylib.
There *has* to be a version of `basic_string,
Allocator>::~basic_string` in the dylib - existing applications depend
upon it. (same for `wchar_t`).
https://
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@
EricWF added a comment.
Has the fix been merged into the 3.9 branch? Does re-adding this attribute mean
that Clang 4.0 is required to build LLVM w/ libc++?
https://reviews.llvm.org/D25624
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http
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
hiraditya created this revision.
hiraditya added reviewers: mclow.lists, EricWF.
hiraditya added subscribers: laxmansole, sebpop, cfe-commits.
Author: laxmansole
Original Differential Revision: https://reviews.llvm.org/D22834
Posting the patch again as the bug https://llvm.org/bu
31 matches
Mail list logo
