<div dir="ltr"><div>I'm probably missing something, but will std::vector move if you mark TrackingMDRef's move ctor noexcept?</div><div><br></div>I ask because throwing `static_assert(std::is_nothrow_move_constructible<llvm::TrackingMDRef>::value, "");` into TrackingMDRef.h fails to compile on my box, unless I explicitly mark TrackingMDRef's move ctor noexcept. IIRC, std::vector has to copy when reallocating unless T's move ctor is noexcept.<div><br><div class="gmail_extra"><div class="gmail_quote">On Wed, Apr 20, 2016 at 2:39 PM, Duncan P. N. Exon Smith via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><br>
> On 2016-Apr-20, at 14:15, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
><br>
>><br>
>> On 2016-Apr-20, at 14:12, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>> wrote:<br>
>><br>
>>><br>
>>> On 2016-Apr-20, at 13:59, Duncan P. N. Exon Smith via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
>>><br>
>>>><br>
>>>> On 2016-Apr-20, at 13:28, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
>>>><br>
>>>><br>
>>>><br>
>>>> On Wed, Apr 20, 2016 at 1:14 PM, Duncan P. N. Exon Smith via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
>>>> Author: dexonsmith<br>
>>>> Date: Wed Apr 20 15:14:09 2016<br>
>>>> New Revision: 266909<br>
>>>><br>
>>>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=266909&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=266909&view=rev</a><br>
>>>> Log:<br>
>>>> IR: Use SmallVector instead of std::vector of TrackingMDRef<br>
>>>><br>
>>>> Don't use std::vector<TrackingMDRef>, since (at least in some versions<br>
>>>> of libc++) std::vector apparently copies values on grow operations<br>
>>>> instead of moving them. Found this when I was temporarily deleting the<br>
>>>> copy constructor for TrackingMDRef to investigate a performance<br>
>>>> bottleneck.<br>
>>>><br>
>>>> Got a build log? That seems unlikely, since we have vectors of unique_ptr in the codebase. Perhaps you used some specific operation that required a copy?<br>
>><br>
>> It may be that it uses copy if available; otherwise move.<br>
>><br>
>>>> I imagine the append you had in the previous patch? Should've been an append with move_iterators? (& maybe that should've been 'assign' instead, I think - in passing)<br>
>>><br>
>>> It's the `emplace_back()` calls.<br>
>><br>
>> Copied in build log for the one in DIBuilder:<br>
>><br>
>>>><br>
>>>> Modified:<br>
>>>> llvm/trunk/include/llvm/IR/DIBuilder.h<br>
>>>> llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp<br>
>>>> llvm/trunk/lib/IR/DIBuilder.cpp<br>
>>>><br>
>>>> Modified: llvm/trunk/include/llvm/IR/DIBuilder.h<br>
>>>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DIBuilder.h?rev=266909&r1=266908&r2=266909&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DIBuilder.h?rev=266909&r1=266908&r2=266909&view=diff</a><br>
>>>> ==============================================================================<br>
>>>> --- llvm/trunk/include/llvm/IR/DIBuilder.h (original)<br>
>>>> +++ llvm/trunk/include/llvm/IR/DIBuilder.h Wed Apr 20 15:14:09 2016<br>
>>>> @@ -51,7 +51,11 @@ namespace llvm {<br>
>>>> bool AllowUnresolvedNodes;<br>
>>>><br>
>>>> /// Each subprogram's preserved local variables.<br>
>>>> - DenseMap<MDNode *, std::vector<TrackingMDNodeRef>> PreservedVariables;<br>
>>>> + ///<br>
>>>> + /// Do not use a std::vector. Some versions of libc++ apparently copy<br>
>>>> + /// instead of move on grow operations, and TrackingMDRef is expensive to<br>
>>>> + /// copy.<br>
>>>> + DenseMap<MDNode *, SmallVector<TrackingMDNodeRef, 1>> PreservedVariables;<br>
>>>><br>
>>>> DIBuilder(const DIBuilder &) = delete;<br>
>>>> void operator=(const DIBuilder &) = delete;<br>
>>>><br>
>>>> Modified: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp<br>
>>>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=266909&r1=266908&r2=266909&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=266909&r1=266908&r2=266909&view=diff</a><br>
>>>> ==============================================================================<br>
>>>> --- llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp (original)<br>
>>>> +++ llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp Wed Apr 20 15:14:09 2016<br>
>>>> @@ -107,7 +107,12 @@ class BitcodeReaderMetadataList {<br>
>>>> bool AnyFwdRefs;<br>
>>>> unsigned MinFwdRef;<br>
>>>> unsigned MaxFwdRef;<br>
>>>> - std::vector<TrackingMDRef> MetadataPtrs;<br>
>>>> +<br>
>>>> + /// Array of metadata references.<br>
>>>> + ///<br>
>>>> + /// Don't use std::vector here. Some versions of libc++ copy (instead of<br>
>>>> + /// move) on resize, and TrackingMDRef is very expensive to copy.<br>
>>>> + SmallVector<TrackingMDRef, 1> MetadataPtrs;<br>
>>>><br>
>>>> LLVMContext &Context;<br>
>>>> public:<br>
>>>><br>
>>>> Modified: llvm/trunk/lib/IR/DIBuilder.cpp<br>
>>>> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DIBuilder.cpp?rev=266909&r1=266908&r2=266909&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DIBuilder.cpp?rev=266909&r1=266908&r2=266909&view=diff</a><br>
>>>> ==============================================================================<br>
>>>> --- llvm/trunk/lib/IR/DIBuilder.cpp (original)<br>
>>>> +++ llvm/trunk/lib/IR/DIBuilder.cpp Wed Apr 20 15:14:09 2016<br>
>>>> @@ -614,7 +614,7 @@ DIGlobalVariable *DIBuilder::createTempG<br>
>>>><br>
>>>> static DILocalVariable *createLocalVariable(<br>
>>>> LLVMContext &VMContext,<br>
>>>> - DenseMap<MDNode *, std::vector<TrackingMDNodeRef>> &PreservedVariables,<br>
>>>> + DenseMap<MDNode *, SmallVector<TrackingMDNodeRef, 1>> &PreservedVariables,<br>
>>>> DIScope *Scope, StringRef Name, unsigned ArgNo, DIFile *File,<br>
>>>> unsigned LineNo, DIType *Ty, bool AlwaysPreserve, unsigned Flags) {<br>
>>>> // FIXME: Why getNonCompileUnitScope()?<br>
>><br>
>> The code is:<br>
>> --<br>
>> PreservedVariables[Fn].emplace_back(Node);<br>
>> --<br>
>><br>
>> Commenting out TrackingMDRef copy constructor and assignment, this compiles with SmallVector.<br>
><br>
><br>
> If I *also* comment out the TypedTrackingMDRef copy constructor and assignment, this compiles with std::vector. But it looks to me like std::vector (at least the version I'm using) prefers the copying versions.<br>
<br>
</div></div>Hmm. printf is fairly convincing. The compile errors are a bug, but it must be while evaluating some type trait. std::vector is still doing the move that we expect. I'll revert shortly.<br>
<br>
This reproduces the compile error for me:<br>
--<br>
#include <vector><br>
<br>
struct NonCopyable {<br>
NonCopyable(int);<br>
NonCopyable(NonCopyable&&);<br>
};<br>
<br>
template <class T> struct Wrapper {<br>
T value;<br>
Wrapper(int i) : value(i) {}<br>
Wrapper(Wrapper &&x) : value(std::move(x.value)) {}<br>
Wrapper(const Wrapper &x) : value(x.value) {}<br>
};<br>
<br>
void foo() {<br>
std::vector<Wrapper<NonCopyable>> vector;<br>
vector.emplace_back(5);<br>
}<br>
--<br>
<br>
Note that I'm not using ToT libc++. Bug may be fixed in ToT?<br>
<div><div><br>
<br>
><br>
>> With std::vector, I get:<br>
>> --<br>
>> $ /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/IR -I/Users/dexonsmith/data/llvm/staging/lib/IR -Iinclude -I/Users/dexonsmith/data/llvm/staging/include -fPIC -fvisibility-inlines-hidden -Wall -W -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wcovered-switch-default -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -std=c++11 -fcolor-diagnostics -O2 -g -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.11.sdk -UNDEBUG -fno-exceptions -fno-rtti -MMD -MT lib/IR/CMakeFiles/LLVMCore.dir/DIBuilder.cpp.o -MF lib/IR/CMakeFiles/LLVMCore.dir/DIBuilder.cpp.o.d -o lib/IR/CMakeFiles/LLVMCore.dir/DIBuilder.cpp.o -c /Users/dexonsmith/data/llvm/staging/lib/IR/DIBuilder.cpp<br>
>> In file included from /Users/dexonsmith/data/llvm/staging/lib/IR/DIBuilder.cpp:14:<br>
>> In file included from /Users/dexonsmith/data/llvm/staging/include/llvm/IR/DIBuilder.h:19:<br>
>> /Users/dexonsmith/data/llvm/staging/include/llvm/IR/TrackingMDRef.h:108:53: error: call to implicitly-deleted copy constructor of 'llvm::TrackingMDRef'<br>
>> TypedTrackingMDRef(const TypedTrackingMDRef &X) : Ref(X.Ref) {}<br>
>> ^ ~~~~~<br>
>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:1731:31: note: in instantiation of member function<br>
>> 'llvm::TypedTrackingMDRef<llvm::MDNode>::TypedTrackingMDRef' requested here<br>
>> ::new((void*)__p) _Up(_VSTD::forward<_Args>(__args)...);<br>
>> ^<br>
>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:1647:18: note: in instantiation of function template specialization<br>
>> 'std::__1::allocator<llvm::TypedTrackingMDRef<llvm::MDNode> >::construct<llvm::TypedTrackingMDRef<llvm::MDNode>, const llvm::TypedTrackingMDRef<llvm::MDNode> &>' requested here<br>
>> {__a.construct(__p, _VSTD::forward<_Args>(__args)...);}<br>
>> ^<br>
>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:1493:14: note: in instantiation of function template specialization<br>
>> 'std::__1::allocator_traits<std::__1::allocator<llvm::TypedTrackingMDRef<llvm::MDNode> > >::__construct<llvm::TypedTrackingMDRef<llvm::MDNode>, const llvm::TypedTrackingMDRef<llvm::MDNode><br>
>> &>' requested here<br>
>> {__construct(__has_construct<allocator_type, _Tp*, _Args...>(),<br>
>> ^<br>
>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:1609:17: note: in instantiation of function template specialization<br>
>> 'std::__1::allocator_traits<std::__1::allocator<llvm::TypedTrackingMDRef<llvm::MDNode> > >::construct<llvm::TypedTrackingMDRef<llvm::MDNode>, const llvm::TypedTrackingMDRef<llvm::MDNode><br>
>> &>' requested here<br>
>> construct(__a, _VSTD::__to_raw_pointer(__end2-1), _VSTD::move_if_noexcept(*--__end1));<br>
>> ^<br>
>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/vector:889:21: note: in instantiation of function template specialization<br>
>> 'std::__1::allocator_traits<std::__1::allocator<llvm::TypedTrackingMDRef<llvm::MDNode> > >::__construct_backward<llvm::TypedTrackingMDRef<llvm::MDNode> *>' requested here<br>
>> __alloc_traits::__construct_backward(this->__alloc(), this->__begin_, this->__end_, __v.__begin_);<br>
>> ^<br>
>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/vector:1627:5: note: in instantiation of member function<br>
>> 'std::__1::vector<llvm::TypedTrackingMDRef<llvm::MDNode>, std::__1::allocator<llvm::TypedTrackingMDRef<llvm::MDNode> > >::__swap_out_circular_buffer' requested here<br>
>> __swap_out_circular_buffer(__v);<br>
>> ^<br>
>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/vector:1646:9: note: in instantiation of function template specialization<br>
>> 'std::__1::vector<llvm::TypedTrackingMDRef<llvm::MDNode>, std::__1::allocator<llvm::TypedTrackingMDRef<llvm::MDNode> > >::__emplace_back_slow_path<llvm::DILocalVariable *&>' requested here<br>
>> __emplace_back_slow_path(_VSTD::forward<_Args>(__args)...);<br>
>> ^<br>
>> /Users/dexonsmith/data/llvm/staging/lib/IR/DIBuilder.cpp:635:28: note: in instantiation of function template specialization 'std::__1::vector<llvm::TypedTrackingMDRef<llvm::MDNode>,<br>
>> std::__1::allocator<llvm::TypedTrackingMDRef<llvm::MDNode> > >::emplace_back<llvm::DILocalVariable *&>' requested here<br>
>> PreservedVariables[Fn].emplace_back(Node);<br>
>> ^<br>
>> /Users/dexonsmith/data/llvm/staging/include/llvm/IR/TrackingMDRef.h:31:3: note: copy constructor is implicitly deleted because 'TrackingMDRef' has a user-declared move constructor<br>
>> TrackingMDRef(TrackingMDRef &&X) : MD(<a href="http://X.MD" rel="noreferrer" target="_blank">X.MD</a>) { retrack(X); }<br>
>> ^<br>
>> 1 error generated.<br>
>> --<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div></div>