[llvm] r266909 - IR: Use SmallVector instead of std::vector of TrackingMDRef

Wed Apr 20 14:12:50 PDT 2016

> 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.

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.
--