[llvm] r266909 - IR: Use SmallVector instead of std::vector of TrackingMDRef
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 20 14:39:19 PDT 2016
> On 2016-Apr-20, at 14:15, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>
>>
>> On 2016-Apr-20, at 14:12, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>
>>>
>>> On 2016-Apr-20, at 13:59, Duncan P. N. Exon Smith via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>>
>>>>
>>>> On 2016-Apr-20, at 13:28, David Blaikie <dblaikie at gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> On Wed, Apr 20, 2016 at 1:14 PM, Duncan P. N. Exon Smith via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>>> Author: dexonsmith
>>>> Date: Wed Apr 20 15:14:09 2016
>>>> New Revision: 266909
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=266909&view=rev
>>>> Log:
>>>> IR: Use SmallVector instead of std::vector of TrackingMDRef
>>>>
>>>> Don't use std::vector<TrackingMDRef>, since (at least in some versions
>>>> of libc++) std::vector apparently copies values on grow operations
>>>> instead of moving them. Found this when I was temporarily deleting the
>>>> copy constructor for TrackingMDRef to investigate a performance
>>>> bottleneck.
>>>>
>>>> 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?
>>
>> It may be that it uses copy if available; otherwise move.
>>
>>>> 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)
>>>
>>> It's the `emplace_back()` calls.
>>
>> Copied in build log for the one in DIBuilder:
>>
>>>>
>>>> Modified:
>>>> llvm/trunk/include/llvm/IR/DIBuilder.h
>>>> llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp
>>>> llvm/trunk/lib/IR/DIBuilder.cpp
>>>>
>>>> Modified: llvm/trunk/include/llvm/IR/DIBuilder.h
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/DIBuilder.h?rev=266909&r1=266908&r2=266909&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/include/llvm/IR/DIBuilder.h (original)
>>>> +++ llvm/trunk/include/llvm/IR/DIBuilder.h Wed Apr 20 15:14:09 2016
>>>> @@ -51,7 +51,11 @@ namespace llvm {
>>>> bool AllowUnresolvedNodes;
>>>>
>>>> /// Each subprogram's preserved local variables.
>>>> - DenseMap<MDNode *, std::vector<TrackingMDNodeRef>> PreservedVariables;
>>>> + ///
>>>> + /// Do not use a std::vector. Some versions of libc++ apparently copy
>>>> + /// instead of move on grow operations, and TrackingMDRef is expensive to
>>>> + /// copy.
>>>> + DenseMap<MDNode *, SmallVector<TrackingMDNodeRef, 1>> PreservedVariables;
>>>>
>>>> DIBuilder(const DIBuilder &) = delete;
>>>> void operator=(const DIBuilder &) = delete;
>>>>
>>>> Modified: llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp?rev=266909&r1=266908&r2=266909&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp (original)
>>>> +++ llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp Wed Apr 20 15:14:09 2016
>>>> @@ -107,7 +107,12 @@ class BitcodeReaderMetadataList {
>>>> bool AnyFwdRefs;
>>>> unsigned MinFwdRef;
>>>> unsigned MaxFwdRef;
>>>> - std::vector<TrackingMDRef> MetadataPtrs;
>>>> +
>>>> + /// Array of metadata references.
>>>> + ///
>>>> + /// Don't use std::vector here. Some versions of libc++ copy (instead of
>>>> + /// move) on resize, and TrackingMDRef is very expensive to copy.
>>>> + SmallVector<TrackingMDRef, 1> MetadataPtrs;
>>>>
>>>> LLVMContext &Context;
>>>> public:
>>>>
>>>> Modified: llvm/trunk/lib/IR/DIBuilder.cpp
>>>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DIBuilder.cpp?rev=266909&r1=266908&r2=266909&view=diff
>>>> ==============================================================================
>>>> --- llvm/trunk/lib/IR/DIBuilder.cpp (original)
>>>> +++ llvm/trunk/lib/IR/DIBuilder.cpp Wed Apr 20 15:14:09 2016
>>>> @@ -614,7 +614,7 @@ DIGlobalVariable *DIBuilder::createTempG
>>>>
>>>> static DILocalVariable *createLocalVariable(
>>>> LLVMContext &VMContext,
>>>> - DenseMap<MDNode *, std::vector<TrackingMDNodeRef>> &PreservedVariables,
>>>> + DenseMap<MDNode *, SmallVector<TrackingMDNodeRef, 1>> &PreservedVariables,
>>>> DIScope *Scope, StringRef Name, unsigned ArgNo, DIFile *File,
>>>> unsigned LineNo, DIType *Ty, bool AlwaysPreserve, unsigned Flags) {
>>>> // FIXME: Why getNonCompileUnitScope()?
>>
>> The code is:
>> --
>> PreservedVariables[Fn].emplace_back(Node);
>> --
>>
>> Commenting out TrackingMDRef copy constructor and assignment, this compiles with SmallVector.
>
>
> 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.
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.
This reproduces the compile error for me:
--
#include <vector>
struct NonCopyable {
NonCopyable(int);
NonCopyable(NonCopyable&&);
};
template <class T> struct Wrapper {
T value;
Wrapper(int i) : value(i) {}
Wrapper(Wrapper &&x) : value(std::move(x.value)) {}
Wrapper(const Wrapper &x) : value(x.value) {}
};
void foo() {
std::vector<Wrapper<NonCopyable>> vector;
vector.emplace_back(5);
}
--
Note that I'm not using ToT libc++. Bug may be fixed in ToT?
>
>> With std::vector, I get:
>> --
>> $ /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
>> In file included from /Users/dexonsmith/data/llvm/staging/lib/IR/DIBuilder.cpp:14:
>> In file included from /Users/dexonsmith/data/llvm/staging/include/llvm/IR/DIBuilder.h:19:
>> /Users/dexonsmith/data/llvm/staging/include/llvm/IR/TrackingMDRef.h:108:53: error: call to implicitly-deleted copy constructor of 'llvm::TrackingMDRef'
>> TypedTrackingMDRef(const TypedTrackingMDRef &X) : Ref(X.Ref) {}
>> ^ ~~~~~
>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:1731:31: note: in instantiation of member function
>> 'llvm::TypedTrackingMDRef<llvm::MDNode>::TypedTrackingMDRef' requested here
>> ::new((void*)__p) _Up(_VSTD::forward<_Args>(__args)...);
>> ^
>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:1647:18: note: in instantiation of function template specialization
>> 'std::__1::allocator<llvm::TypedTrackingMDRef<llvm::MDNode> >::construct<llvm::TypedTrackingMDRef<llvm::MDNode>, const llvm::TypedTrackingMDRef<llvm::MDNode> &>' requested here
>> {__a.construct(__p, _VSTD::forward<_Args>(__args)...);}
>> ^
>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:1493:14: note: in instantiation of function template specialization
>> 'std::__1::allocator_traits<std::__1::allocator<llvm::TypedTrackingMDRef<llvm::MDNode> > >::__construct<llvm::TypedTrackingMDRef<llvm::MDNode>, const llvm::TypedTrackingMDRef<llvm::MDNode>
>> &>' requested here
>> {__construct(__has_construct<allocator_type, _Tp*, _Args...>(),
>> ^
>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/memory:1609:17: note: in instantiation of function template specialization
>> 'std::__1::allocator_traits<std::__1::allocator<llvm::TypedTrackingMDRef<llvm::MDNode> > >::construct<llvm::TypedTrackingMDRef<llvm::MDNode>, const llvm::TypedTrackingMDRef<llvm::MDNode>
>> &>' requested here
>> construct(__a, _VSTD::__to_raw_pointer(__end2-1), _VSTD::move_if_noexcept(*--__end1));
>> ^
>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/vector:889:21: note: in instantiation of function template specialization
>> 'std::__1::allocator_traits<std::__1::allocator<llvm::TypedTrackingMDRef<llvm::MDNode> > >::__construct_backward<llvm::TypedTrackingMDRef<llvm::MDNode> *>' requested here
>> __alloc_traits::__construct_backward(this->__alloc(), this->__begin_, this->__end_, __v.__begin_);
>> ^
>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/vector:1627:5: note: in instantiation of member function
>> 'std::__1::vector<llvm::TypedTrackingMDRef<llvm::MDNode>, std::__1::allocator<llvm::TypedTrackingMDRef<llvm::MDNode> > >::__swap_out_circular_buffer' requested here
>> __swap_out_circular_buffer(__v);
>> ^
>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/vector:1646:9: note: in instantiation of function template specialization
>> 'std::__1::vector<llvm::TypedTrackingMDRef<llvm::MDNode>, std::__1::allocator<llvm::TypedTrackingMDRef<llvm::MDNode> > >::__emplace_back_slow_path<llvm::DILocalVariable *&>' requested here
>> __emplace_back_slow_path(_VSTD::forward<_Args>(__args)...);
>> ^
>> /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>,
>> std::__1::allocator<llvm::TypedTrackingMDRef<llvm::MDNode> > >::emplace_back<llvm::DILocalVariable *&>' requested here
>> PreservedVariables[Fn].emplace_back(Node);
>> ^
>> /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
>> TrackingMDRef(TrackingMDRef &&X) : MD(X.MD) { retrack(X); }
>> ^
>> 1 error generated.
>> --
More information about the llvm-commits
mailing list