[llvm] r206857 - blockfreq: Use pointers to loops instead of an index

[David Blaikie]

On Mon, Apr 21, 2014 at 8:31 PM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
> Author: dexonsmith
> Date: Mon Apr 21 22:31:37 2014
> New Revision: 206857
>
> URL: http://llvm.org/viewvc/llvm-project?rev=206857&view=rev
> Log:
> blockfreq: Use pointers to loops instead of an index
>
> Store pointers directly to loops inside the nodes.  This could have been
> done without changing the type stored in `std::vector<>`.  However,
> rather than computing the number of loops before constructing them
> (which `LoopInfo` doesn't provide directly), I've switched to a
> `vector<unique_ptr<LoopData>>`.

Why not use a std::list<LoopData>? If you're going to incur a heap
allocation for every node anyway, this at least hides it as an
implementation detail (& you can include a comment explaining that
you're taking pointers to these things and so you need the iterator
(non)invalidation semantics of list over vector)

I've made a change from vector<(owning)T*> to list<T> in one or two
places today for just this reason.

>
> This adds some heap overhead, but the number of loops is typically
> small.
>
> Modified:
>     llvm/trunk/include/llvm/Analysis/BlockFrequencyInfoImpl.h
>     llvm/trunk/lib/Analysis/BlockFrequencyInfoImpl.cpp
>
> Modified: llvm/trunk/include/llvm/Analysis/BlockFrequencyInfoImpl.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/BlockFrequencyInfoImpl.h?rev=206857&r1=206856&r2=206857&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Analysis/BlockFrequencyInfoImpl.h (original)
> +++ llvm/trunk/include/llvm/Analysis/BlockFrequencyInfoImpl.h Mon Apr 21 22:31:37 2014
> @@ -945,28 +945,30 @@ public:
>      typedef SmallVector<std::pair<BlockNode, BlockMass>, 4> ExitMap;
>      typedef SmallVector<BlockNode, 4> MemberList;
>      BlockNode Header;       ///< Header.
> +    bool IsPackaged;        ///< Whether this has been packaged.
>      ExitMap Exits;          ///< Successor edges (and weights).
>      MemberList Members;     ///< Members of the loop.
>      BlockMass BackedgeMass; ///< Mass returned to loop header.
>      BlockMass Mass;
>      Float Scale;
>
> -    LoopData(const BlockNode &Header) : Header(Header) {}
> +    LoopData(const BlockNode &Header) : Header(Header), IsPackaged(false) {}
>    };
>
>    /// \brief Index of loop information.
>    struct WorkingData {
> +    LoopData *Loop;           ///< The loop this block is the header of.
>      BlockNode ContainingLoop; ///< The block whose loop this block is inside.
> -    uint32_t LoopIndex;       ///< Index into PackagedLoops.
>      bool IsPackaged;          ///< Has ContainingLoop been packaged up?
> -    bool IsAPackage;          ///< Has this block's loop been packaged up?
>      BlockMass Mass;           ///< Mass distribution from the entry block.
>
> -    WorkingData()
> -        : LoopIndex(UINT32_MAX), IsPackaged(false), IsAPackage(false) {}
> +    WorkingData() : Loop(nullptr), IsPackaged(false) {}
>
>      bool hasLoopHeader() const { return ContainingLoop.isValid(); }
> -    bool isLoopHeader() const { return LoopIndex != UINT32_MAX; }
> +    bool isLoopHeader() const { return Loop; }
> +
> +    /// \brief Has this block's loop been packaged up?
> +    bool isAPackage() const { return Loop && Loop->IsPackaged; }
>    };
>
>    /// \brief Unscaled probability weight.
> @@ -1040,7 +1042,7 @@ public:
>    std::vector<WorkingData> Working;
>
>    /// \brief Indexed information about packaged loops.
> -  std::vector<LoopData> PackagedLoops;
> +  std::vector<std::unique_ptr<LoopData>> PackagedLoops;
>
>    /// \brief Add all edges out of a packaged loop to the distribution.
>    ///
> @@ -1061,9 +1063,8 @@ public:
>
>    LoopData &getLoopPackage(const BlockNode &Head) {
>      assert(Head.Index < Working.size());
> -    size_t Index = Working[Head.Index].LoopIndex;
> -    assert(Index < PackagedLoops.size());
> -    return PackagedLoops[Index];
> +    assert(Working[Head.Index].Loop != nullptr);
> +    return *Working[Head.Index].Loop;
>    }
>
>    /// \brief Distribute mass according to a distribution.
> @@ -1428,8 +1429,8 @@ template <class BT> void BlockFrequencyI
>      BlockNode Header = getNode(Loop->getHeader());
>      assert(Header.isValid());
>
> -    Working[Header.Index].LoopIndex = PackagedLoops.size();
> -    PackagedLoops.emplace_back(Header);
> +    PackagedLoops.emplace_back(new LoopData(Header));
> +    Working[Header.Index].Loop = PackagedLoops.back().get();
>      DEBUG(dbgs() << " - loop = " << getBlockName(Header) << "\n");
>    }
>
> @@ -1452,7 +1453,7 @@ template <class BT> void BlockFrequencyI
>      assert(HeaderData.isLoopHeader());
>
>      Working[Index].ContainingLoop = Header;
> -    PackagedLoops[HeaderData.LoopIndex].Members.push_back(Index);
> +    HeaderData.Loop->Members.push_back(Index);
>      DEBUG(dbgs() << " - loop = " << getBlockName(Header)
>                   << ": member = " << getBlockName(Index) << "\n");
>    }
> @@ -1460,7 +1461,7 @@ template <class BT> void BlockFrequencyI
>
>  template <class BT> void BlockFrequencyInfoImpl<BT>::computeMassInLoops() {
>    // Visit loops with the deepest first, and the top-level loops last.
> -  for (auto L = PackagedLoops.rbegin(), LE = PackagedLoops.rend(); L != LE; ++L)
> +  for (const auto &L : make_range(PackagedLoops.rbegin(), PackagedLoops.rend()))
>      computeMassInLoop(L->Header);
>  }
>
>
> Modified: llvm/trunk/lib/Analysis/BlockFrequencyInfoImpl.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/BlockFrequencyInfoImpl.cpp?rev=206857&r1=206856&r2=206857&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Analysis/BlockFrequencyInfoImpl.cpp (original)
> +++ llvm/trunk/lib/Analysis/BlockFrequencyInfoImpl.cpp Mon Apr 21 22:31:37 2014
> @@ -602,7 +602,7 @@ void BlockFrequencyInfoImplBase::clear()
>    // does not actually clear heap storage.
>    std::vector<FrequencyData>().swap(Freqs);
>    std::vector<WorkingData>().swap(Working);
> -  std::vector<LoopData>().swap(PackagedLoops);
> +  std::vector<std::unique_ptr<LoopData>>().swap(PackagedLoops);
>  }
>
>  /// \brief Clear all memory not needed downstream.
> @@ -646,7 +646,7 @@ static BlockMass &getPackageMass(BlockFr
>                                   const BlockNode &Node) {
>    assert(Node.isValid());
>    assert(!BFI.Working[Node.Index].IsPackaged);
> -  if (!BFI.Working[Node.Index].IsAPackage)
> +  if (!BFI.Working[Node.Index].isAPackage())
>      return BFI.Working[Node.Index].Mass;
>
>    return BFI.getLoopPackage(Node).Mass;
> @@ -744,8 +744,9 @@ void BlockFrequencyInfoImplBase::compute
>  /// \brief Package up a loop.
>  void BlockFrequencyInfoImplBase::packageLoop(const BlockNode &LoopHead) {
>    DEBUG(dbgs() << "packaging-loop: " << getBlockName(LoopHead) << "\n");
> -  Working[LoopHead.Index].IsAPackage = true;
> -  for (const BlockNode &M : getLoopPackage(LoopHead).Members) {
> +  auto &PackagedLoop = getLoopPackage(LoopHead);
> +  PackagedLoop.IsPackaged = true;
> +  for (const BlockNode &M : PackagedLoop.Members) {
>      DEBUG(dbgs() << " - node: " << getBlockName(M.Index) << "\n");
>      Working[M.Index].IsPackaged = true;
>    }
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits