[llvm] r225966 - [PM] Make DominatorTrees (corectly) movable so that we can move them

[David Blaikie]

On Wed, Jan 14, 2015 at 2:07 AM, Chandler Carruth <chandlerc at gmail.com>
wrote:

> Author: chandlerc
> Date: Wed Jan 14 04:07:19 2015
> New Revision: 225966
>
> URL: http://llvm.org/viewvc/llvm-project?rev=225966&view=rev
> Log:
> [PM] Make DominatorTrees (corectly) movable so that we can move them
> into the new pass manager's analysis cache which stores results
> by-value.
>
> Technically speaking, the dom trees were originally not movable but
> copyable! This, unsurprisingly, didn't work at all -- the copy was
> shallow and just resulted in rampant memory corruption. This change
> explicitly forbids copying (as it would need to be a deep copy) and
> makes them explicitly movable with the unsurprising boiler plate to
> member-wise move them because we can't rely on MSVC to generate this
> code for us. =/
>
> Modified:
>     llvm/trunk/include/llvm/IR/Dominators.h
>     llvm/trunk/include/llvm/Support/GenericDomTree.h
>
> Modified: llvm/trunk/include/llvm/IR/Dominators.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Dominators.h?rev=225966&r1=225965&r2=225966&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/IR/Dominators.h (original)
> +++ llvm/trunk/include/llvm/IR/Dominators.h Wed Jan 14 04:07:19 2015
> @@ -69,6 +69,13 @@ public:
>
>    DominatorTree() : DominatorTreeBase<BasicBlock>(false) {}
>
> +  DominatorTree(DominatorTree &&Arg)
> +      : Base(std::move(static_cast<Base &>(Arg))) {}
> +  DominatorTree &operator=(DominatorTree &&RHS) {
> +    Base::operator=(std::move(static_cast<Base &>(RHS)));
> +    return *this;
> +  }
> +
>    /// \brief Returns *false* if the other dominator tree matches this
> dominator
>    /// tree.
>    inline bool compare(const DominatorTree &Other) const {
>
> Modified: llvm/trunk/include/llvm/Support/GenericDomTree.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/GenericDomTree.h?rev=225966&r1=225965&r2=225966&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/Support/GenericDomTree.h (original)
> +++ llvm/trunk/include/llvm/Support/GenericDomTree.h Wed Jan 14 04:07:19
> 2015
> @@ -34,9 +34,20 @@ namespace llvm {
>  template <class NodeT> class DominatorBase {
>  protected:
>    std::vector<NodeT *> Roots;
> -  const bool IsPostDominators;
> +  bool IsPostDominators;
>    explicit DominatorBase(bool isPostDom)
>        : Roots(), IsPostDominators(isPostDom) {}
> +  DominatorBase(DominatorBase &&Arg)
> +      : Roots(std::move(Arg.Roots)),
> +        IsPostDominators(std::move(Arg.IsPostDominators)) {
> +    Arg.Roots.clear();
> +  }
> +  DominatorBase &operator=(DominatorBase &&RHS) {
> +    Roots = std::move(RHS.Roots);
> +    IsPostDominators = std::move(RHS.IsPostDominators);
> +    RHS.Roots.clear();
> +    return *this;
> +  }
>
>  public:
>    /// getRoots - Return the root blocks of the current CFG.  This may
> include
> @@ -171,6 +182,9 @@ void Calculate(DominatorTreeBase<typenam
>  /// This class is a generic template over graph nodes. It is instantiated
> for
>  /// various graphs in the LLVM IR or in the code generator.
>  template <class NodeT> class DominatorTreeBase : public
> DominatorBase<NodeT> {
> +  DominatorTreeBase(const DominatorTreeBase &) LLVM_DELETED_FUNCTION;
> +  DominatorTreeBase &operator=(const DominatorTreeBase &)
> LLVM_DELETED_FUNCTION;
>

This should be implicit, since it has a base that's non-copyable.

But, yes, since the non-copyability only comes from the need for explicit
move operations which only comes from MSVC's limitations (& will hopefully
be removed once we drop support for whatever versions of MSVC don't create
implicit move ops)... - not sure if it makes more sense to put it here, or
up in the base which owns the container of nodes (but the base doesn't know
what kind of nodes they are/doesn't require those nodes to point to other
nodes, etc, which is where the non-copyability comes in)

Sorry, kind of rambling, maybe chat about it over lunch or something.

- David


> +
>    bool dominatedBySlowTreeWalk(const DomTreeNodeBase<NodeT> *A,
>                                 const DomTreeNodeBase<NodeT> *B) const {
>      assert(A != B);
> @@ -183,6 +197,18 @@ template <class NodeT> class DominatorTr
>      return IDom != nullptr;
>    }
>
> +  /// \brief Wipe this tree's state without releasing any resources.
> +  ///
> +  /// This is essentially a post-move helper only. It leaves the object
> in an
> +  /// assignable and destroyable state, but otherwise invalid.
> +  void wipe() {
> +    DomTreeNodes.clear();
> +    IDoms.clear();
> +    Vertex.clear();
> +    Info.clear();
> +    RootNode = nullptr;
> +  }
> +
>  protected:
>    typedef DenseMap<NodeT *, DomTreeNodeBase<NodeT> *> DomTreeNodeMapType;
>    DomTreeNodeMapType DomTreeNodes;
> @@ -290,6 +316,30 @@ public:
>        : DominatorBase<NodeT>(isPostDom), DFSInfoValid(false),
> SlowQueries(0) {}
>    virtual ~DominatorTreeBase() { reset(); }
>
> +  DominatorTreeBase(DominatorTreeBase &&Arg)
> +      : DominatorBase<NodeT>(
> +            std::move(static_cast<DominatorBase<NodeT> &>(Arg))),
> +        DomTreeNodes(std::move(Arg.DomTreeNodes)),
> +        RootNode(std::move(Arg.RootNode)),
> +        DFSInfoValid(std::move(Arg.DFSInfoValid)),
> +        SlowQueries(std::move(Arg.SlowQueries)),
> IDoms(std::move(Arg.IDoms)),
> +        Vertex(std::move(Arg.Vertex)), Info(std::move(Arg.Info)) {
> +    Arg.wipe();
> +  }
> +  DominatorTreeBase &operator=(DominatorTreeBase &&RHS) {
> +    DominatorBase<NodeT>::operator=(
> +        std::move(static_cast<DominatorBase<NodeT> &>(RHS)));
> +    DomTreeNodes = std::move(RHS.DomTreeNodes);
> +    RootNode = std::move(RHS.RootNode);
> +    DFSInfoValid = std::move(RHS.DFSInfoValid);
> +    SlowQueries = std::move(RHS.SlowQueries);
> +    IDoms = std::move(RHS.IDoms);
> +    Vertex = std::move(RHS.Vertex);
> +    Info = std::move(RHS.Info);
> +    RHS.wipe();
> +    return *this;
> +  }
> +
>    /// compare - Return false if the other dominator tree base matches this
>    /// dominator tree base. Otherwise return true.
>    bool compare(const DominatorTreeBase &Other) const {
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150114/d774527c/attachment.html>