[llvm] r266909 - IR: Use SmallVector instead of std::vector of TrackingMDRef
George Burgess IV via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 20 14:44:49 PDT 2016
I'm probably missing something, but will std::vector move if you mark
TrackingMDRef's move ctor noexcept?
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.
On Wed, Apr 20, 2016 at 2:39 PM, Duncan P. N. Exon Smith via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
>
> > 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.
> >> --
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160420/f96acadd/attachment.html>
More information about the llvm-commits
mailing list