[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 16:38:55 PDT 2016


> On 2016-Apr-20, at 16:20, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Wed, Apr 20, 2016 at 3:36 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> > On 2016-Apr-20, at 14:44, George Burgess IV <george.burgess.iv at gmail.com> wrote:
> >
> > 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.
> 
> Yes, that's it.  Good eye.
> 
> I filed PR27442 on libc++ and then moved it to clang:
>   https://llvm.org/bugs/show_bug.cgi?id=27442
> 
> Does this mean that std::vector pessimizes all of our data structures?
> 
> > 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 actually have a fever right now, so I'm probably not to be trusted at all.  But printf shouldn't have been convincing, since I only ran it on a tiny IR example.  This wasn't just something I fixed by inspection.  I actually saw TrackingMDRef copy constructors being called from std::vector<TrackignMDRef>::emplace_back.
> 
> > I'll revert shortly.
> 
> And so I'm not going to revert until we have a new direction that will avoid the copying.
> 
> Options:
> 
>  1. Delete TrackingMDRef(const TrackingMDRef&).  This would be pretty annoying, but would force people to do the right thing.  During graph construction, the copy constructor is expensive and should be used only when necessary.  (The move constructor isn't exactly cheap, but it's cheaper.)
> 
>  2. Make std::vector on non-trival-copy illegal in our codebase.  In that case, my patch was correct.
> 
>  3. Add a noexcept specification to TrackingMDRef(TrackingMDRef &&).
> 
> David, you were talking over IRC (before you left) about using SFINAE, but I think we were talking past each other (my fault: I couldn't quite remember the original motivation when you sent your review here...). 
> 
> Nah, think that was me - I was rushed/not listening to you properly. Sorry about that.
>  
> My root problem is that std::vector::emplace_back is choosing to copy TrackingMDRef even though it's expensive to do so.  
> 
> I think we'd need to understand how that happens at runtime - so far as I understand it should only happen if an exception is thrown and std::vector is trying to rollback the transaction to provide the strong exception guarantee. If no throws happen, I don't think the copy ctor should ever actually be invoked...  

The assertion I added (which fired, and caused the investigation) required some patches I haven't committed yet.  I should be able to reproduce later today or tomorrow sometime so we can dig deeper.

> There's no compile error to SFINAE on.  The compile errors only came up when I was investigating the cause of the copies, commenting out code left and right.



More information about the llvm-commits mailing list