[llvm] r278961 - [GenericDomTree] Change GenericDomTree to use NodeRef in GraphTraits. NFC.
Vedant Kumar via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 19 16:04:41 PDT 2016
Hi Tim,
This commit breaks clang-3.8 and gcc 5.4.0 on linux:
/root/llvm/include/llvm/Support/GenericDomTree.h:753:35: error: cannot initialize a parameter of type 'NodeType *' (aka 'clang::CFGBlock *') with an rvalue of type
'clang::CFGBlock **'
if (TraitsTy::child_begin(&*I) == TraitsTy::child_end(&*I))
^~~
/root/llvm/tools/clang/include/clang/Analysis/Analyses/Dominators.h:85:9: note: in instantiation of function template specialization
'llvm::DominatorTreeBase<clang::CFGBlock>::recalculate<clang::CFG>' requested here
DT->recalculate(*cfg);
I don't know what the policy is on supported older releases of clang, but it'd be nice to keep this working. Could you take a look?
thanks,
vedant
> On Aug 17, 2016, at 1:01 PM, Tim Shen via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
> Author: timshen
> Date: Wed Aug 17 15:01:58 2016
> New Revision: 278961
>
> URL: http://llvm.org/viewvc/llvm-project?rev=278961&view=rev
> Log:
> [GenericDomTree] Change GenericDomTree to use NodeRef in GraphTraits. NFC.
>
> Summary:
> Looking at the implementation, GenericDomTree has more specific
> requirements on NodeRef, e.g. NodeRefObject->getParent() should compile,
> and NodeRef should be a pointer. We can remove the pointer requirement,
> but it seems to have little gain, given the limited use cases.
>
> Also changed GraphTraits<Inverse<Inverse<T>> to be more accurate.
>
> Reviewers: dblaikie, chandlerc
>
> Subscribers: llvm-commits
>
> Differential Revision: https://reviews.llvm.org/D23593
>
> Modified:
> llvm/trunk/include/llvm/ADT/GraphTraits.h
> llvm/trunk/include/llvm/IR/Dominators.h
> llvm/trunk/include/llvm/Support/GenericDomTree.h
> llvm/trunk/include/llvm/Support/GenericDomTreeConstruction.h
> llvm/trunk/lib/IR/Dominators.cpp
>
> Modified: llvm/trunk/include/llvm/ADT/GraphTraits.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/GraphTraits.h?rev=278961&r1=278960&r2=278961&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/ADT/GraphTraits.h (original)
> +++ llvm/trunk/include/llvm/ADT/GraphTraits.h Wed Aug 17 15:01:58 2016
> @@ -88,23 +88,7 @@ struct Inverse {
>
> // Provide a partial specialization of GraphTraits so that the inverse of an
> // inverse falls back to the original graph.
> -template<class T>
> -struct GraphTraits<Inverse<Inverse<T> > > {
> - typedef typename GraphTraits<T>::NodeType NodeType;
> - typedef typename GraphTraits<T>::ChildIteratorType ChildIteratorType;
> -
> - static NodeType *getEntryNode(Inverse<Inverse<T> > *G) {
> - return GraphTraits<T>::getEntryNode(G->Graph.Graph);
> - }
> -
> - static ChildIteratorType child_begin(NodeType* N) {
> - return GraphTraits<T>::child_begin(N);
> - }
> -
> - static ChildIteratorType child_end(NodeType* N) {
> - return GraphTraits<T>::child_end(N);
> - }
> -};
> +template <class T> struct GraphTraits<Inverse<Inverse<T>>> : GraphTraits<T> {};
>
> } // End llvm namespace
>
>
> Modified: llvm/trunk/include/llvm/IR/Dominators.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/Dominators.h?rev=278961&r1=278960&r2=278961&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/IR/Dominators.h (original)
> +++ llvm/trunk/include/llvm/IR/Dominators.h Wed Aug 17 15:01:58 2016
> @@ -33,9 +33,9 @@ extern template class DomTreeNodeBase<Ba
> extern template class DominatorTreeBase<BasicBlock>;
>
> extern template void Calculate<Function, BasicBlock *>(
> - DominatorTreeBase<GraphTraits<BasicBlock *>::NodeType> &DT, Function &F);
> + DominatorTreeBaseByGraphTraits<GraphTraits<BasicBlock *>> &DT, Function &F);
> extern template void Calculate<Function, Inverse<BasicBlock *>>(
> - DominatorTreeBase<GraphTraits<Inverse<BasicBlock *>>::NodeType> &DT,
> + DominatorTreeBaseByGraphTraits<GraphTraits<Inverse<BasicBlock *>>> &DT,
> Function &F);
>
> typedef DomTreeNodeBase<BasicBlock> DomTreeNode;
>
> Modified: llvm/trunk/include/llvm/Support/GenericDomTree.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/GenericDomTree.h?rev=278961&r1=278960&r2=278961&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Support/GenericDomTree.h (original)
> +++ llvm/trunk/include/llvm/Support/GenericDomTree.h Wed Aug 17 15:01:58 2016
> @@ -13,6 +13,12 @@
> /// dominance queries on the CFG, but is fully generic w.r.t. the underlying
> /// graph types.
> ///
> +/// Unlike ADT/* graph algorithms, generic dominator tree has more reuiqrement
> +/// on the graph's NodeRef. The NodeRef should be a pointer and, depending on
> +/// the implementation, e.g. NodeRef->getParent() return the parent node.
> +///
> +/// FIXME: Maybe GenericDomTree needs a TreeTraits, instead of GraphTraits.
> +///
> //===----------------------------------------------------------------------===//
>
> #ifndef LLVM_SUPPORT_GENERICDOMTREE_H
> @@ -30,6 +36,23 @@
>
> namespace llvm {
>
> +template <class NodeT> class DominatorTreeBase;
> +
> +namespace detail {
> +
> +template <typename GT> struct DominatorTreeBaseTraits {
> + static_assert(std::is_pointer<typename GT::NodeRef>::value,
> + "Currently NodeRef must be a pointer type.");
> + using type = DominatorTreeBase<
> + typename std::remove_pointer<typename GT::NodeRef>::type>;
> +};
> +
> +} // End namespace detail
> +
> +template <typename GT>
> +using DominatorTreeBaseByGraphTraits =
> + typename detail::DominatorTreeBaseTraits<GT>::type;
> +
> /// \brief Base class that other, more interesting dominator analyses
> /// inherit from.
> template <class NodeT> class DominatorBase {
> @@ -62,7 +85,6 @@ public:
> bool isPostDominator() const { return IsPostDominators; }
> };
>
> -template <class NodeT> class DominatorTreeBase;
> struct PostDominatorTree;
>
> /// \brief Base class for the actual dominator tree node.
> @@ -177,8 +199,7 @@ void PrintDomTree(const DomTreeNodeBase<
>
> // The calculate routine is provided in a separate header but referenced here.
> template <class FuncT, class N>
> -void Calculate(DominatorTreeBase<typename GraphTraits<N>::NodeType> &DT,
> - FuncT &F);
> +void Calculate(DominatorTreeBaseByGraphTraits<GraphTraits<N>> &DT, FuncT &F);
>
> /// \brief Core dominator tree base class.
> ///
> @@ -251,14 +272,14 @@ protected:
> // NewBB is split and now it has one successor. Update dominator tree to
> // reflect this change.
> template <class N, class GraphT>
> - void Split(DominatorTreeBase<typename GraphT::NodeType> &DT,
> - typename GraphT::NodeType *NewBB) {
> + void Split(DominatorTreeBaseByGraphTraits<GraphT> &DT,
> + typename GraphT::NodeRef NewBB) {
> assert(std::distance(GraphT::child_begin(NewBB),
> GraphT::child_end(NewBB)) == 1 &&
> "NewBB should have a single successor!");
> - typename GraphT::NodeType *NewBBSucc = *GraphT::child_begin(NewBB);
> + typename GraphT::NodeRef NewBBSucc = *GraphT::child_begin(NewBB);
>
> - std::vector<typename GraphT::NodeType *> PredBlocks;
> + std::vector<typename GraphT::NodeRef> PredBlocks;
> typedef GraphTraits<Inverse<N>> InvTraits;
> for (typename InvTraits::ChildIteratorType
> PI = InvTraits::child_begin(NewBB),
> @@ -273,7 +294,7 @@ protected:
> PI = InvTraits::child_begin(NewBBSucc),
> E = InvTraits::child_end(NewBBSucc);
> PI != E; ++PI) {
> - typename InvTraits::NodeType *ND = *PI;
> + typename InvTraits::NodeRef ND = *PI;
> if (ND != NewBB && !DT.dominates(NewBBSucc, ND) &&
> DT.isReachableFromEntry(ND)) {
> NewBBDominatesNewBBSucc = false;
> @@ -627,18 +648,17 @@ public:
>
> protected:
> template <class GraphT>
> - friend typename GraphT::NodeType *
> - Eval(DominatorTreeBase<typename GraphT::NodeType> &DT,
> - typename GraphT::NodeType *V, unsigned LastLinked);
> + friend typename GraphT::NodeRef
> + Eval(DominatorTreeBaseByGraphTraits<GraphT> &DT, typename GraphT::NodeRef V,
> + unsigned LastLinked);
>
> template <class GraphT>
> - friend unsigned DFSPass(DominatorTreeBase<typename GraphT::NodeType> &DT,
> - typename GraphT::NodeType *V, unsigned N);
> + friend unsigned DFSPass(DominatorTreeBaseByGraphTraits<GraphT> &DT,
> + typename GraphT::NodeRef V, unsigned N);
>
> template <class FuncT, class N>
> - friend void
> - Calculate(DominatorTreeBase<typename GraphTraits<N>::NodeType> &DT, FuncT &F);
> -
> + friend void Calculate(DominatorTreeBaseByGraphTraits<GraphTraits<N>> &DT,
> + FuncT &F);
>
> DomTreeNodeBase<NodeT> *getNodeForBlock(NodeT *BB) {
> if (DomTreeNodeBase<NodeT> *Node = getNode(BB))
>
> Modified: llvm/trunk/include/llvm/Support/GenericDomTreeConstruction.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Support/GenericDomTreeConstruction.h?rev=278961&r1=278960&r2=278961&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Support/GenericDomTreeConstruction.h (original)
> +++ llvm/trunk/include/llvm/Support/GenericDomTreeConstruction.h Wed Aug 17 15:01:58 2016
> @@ -29,9 +29,9 @@
>
> namespace llvm {
>
> -template<class GraphT>
> -unsigned DFSPass(DominatorTreeBase<typename GraphT::NodeType>& DT,
> - typename GraphT::NodeType* V, unsigned N) {
> +template <class GraphT>
> +unsigned DFSPass(DominatorTreeBaseByGraphTraits<GraphT> &DT,
> + typename GraphT::NodeRef V, unsigned N) {
> // This is more understandable as a recursive algorithm, but we can't use the
> // recursive algorithm due to stack depth issues. Keep it here for
> // documentation purposes.
> @@ -52,15 +52,16 @@ unsigned DFSPass(DominatorTreeBase<typen
> #else
> bool IsChildOfArtificialExit = (N != 0);
>
> - SmallVector<std::pair<typename GraphT::NodeType*,
> - typename GraphT::ChildIteratorType>, 32> Worklist;
> + SmallVector<
> + std::pair<typename GraphT::NodeRef, typename GraphT::ChildIteratorType>,
> + 32>
> + Worklist;
> Worklist.push_back(std::make_pair(V, GraphT::child_begin(V)));
> while (!Worklist.empty()) {
> - typename GraphT::NodeType* BB = Worklist.back().first;
> + typename GraphT::NodeRef BB = Worklist.back().first;
> typename GraphT::ChildIteratorType NextSucc = Worklist.back().second;
>
> - typename DominatorTreeBase<typename GraphT::NodeType>::InfoRec &BBInfo =
> - DT.Info[BB];
> + auto &BBInfo = DT.Info[BB];
>
> // First time we visited this BB?
> if (NextSucc == GraphT::child_begin(BB)) {
> @@ -89,10 +90,9 @@ unsigned DFSPass(DominatorTreeBase<typen
> ++Worklist.back().second;
>
> // Visit the successor next, if it isn't already visited.
> - typename GraphT::NodeType* Succ = *NextSucc;
> + typename GraphT::NodeRef Succ = *NextSucc;
>
> - typename DominatorTreeBase<typename GraphT::NodeType>::InfoRec &SuccVInfo =
> - DT.Info[Succ];
> + auto &SuccVInfo = DT.Info[Succ];
> if (SuccVInfo.Semi == 0) {
> SuccVInfo.Parent = BBDFSNum;
> Worklist.push_back(std::make_pair(Succ, GraphT::child_begin(Succ)));
> @@ -103,25 +103,23 @@ unsigned DFSPass(DominatorTreeBase<typen
> }
>
> template <class GraphT>
> -typename GraphT::NodeType *
> -Eval(DominatorTreeBase<typename GraphT::NodeType> &DT,
> - typename GraphT::NodeType *VIn, unsigned LastLinked) {
> - typename DominatorTreeBase<typename GraphT::NodeType>::InfoRec &VInInfo =
> - DT.Info[VIn];
> +typename GraphT::NodeRef Eval(DominatorTreeBaseByGraphTraits<GraphT> &DT,
> + typename GraphT::NodeRef VIn,
> + unsigned LastLinked) {
> + auto &VInInfo = DT.Info[VIn];
> if (VInInfo.DFSNum < LastLinked)
> return VIn;
>
> - SmallVector<typename GraphT::NodeType*, 32> Work;
> - SmallPtrSet<typename GraphT::NodeType*, 32> Visited;
> + SmallVector<typename GraphT::NodeRef, 32> Work;
> + SmallPtrSet<typename GraphT::NodeRef, 32> Visited;
>
> if (VInInfo.Parent >= LastLinked)
> Work.push_back(VIn);
>
> while (!Work.empty()) {
> - typename GraphT::NodeType* V = Work.back();
> - typename DominatorTreeBase<typename GraphT::NodeType>::InfoRec &VInfo =
> - DT.Info[V];
> - typename GraphT::NodeType* VAncestor = DT.Vertex[VInfo.Parent];
> + typename GraphT::NodeRef V = Work.back();
> + auto &VInfo = DT.Info[V];
> + typename GraphT::NodeRef VAncestor = DT.Vertex[VInfo.Parent];
>
> // Process Ancestor first
> if (Visited.insert(VAncestor).second && VInfo.Parent >= LastLinked) {
> @@ -134,10 +132,9 @@ Eval(DominatorTreeBase<typename GraphT::
> if (VInfo.Parent < LastLinked)
> continue;
>
> - typename DominatorTreeBase<typename GraphT::NodeType>::InfoRec &VAInfo =
> - DT.Info[VAncestor];
> - typename GraphT::NodeType* VAncestorLabel = VAInfo.Label;
> - typename GraphT::NodeType* VLabel = VInfo.Label;
> + auto &VAInfo = DT.Info[VAncestor];
> + typename GraphT::NodeRef VAncestorLabel = VAInfo.Label;
> + typename GraphT::NodeRef VLabel = VInfo.Label;
> if (DT.Info[VAncestorLabel].Semi < DT.Info[VLabel].Semi)
> VInfo.Label = VAncestorLabel;
> VInfo.Parent = VAInfo.Parent;
> @@ -146,16 +143,18 @@ Eval(DominatorTreeBase<typename GraphT::
> return VInInfo.Label;
> }
>
> -template<class FuncT, class NodeT>
> -void Calculate(DominatorTreeBase<typename GraphTraits<NodeT>::NodeType>& DT,
> - FuncT& F) {
> +template <class FuncT, class NodeT>
> +void Calculate(DominatorTreeBaseByGraphTraits<GraphTraits<NodeT>> &DT,
> + FuncT &F) {
> typedef GraphTraits<NodeT> GraphT;
> + static_assert(std::is_pointer<typename GraphT::NodeRef>::value,
> + "NodeRef should be pointer type");
> + typedef typename std::remove_pointer<typename GraphT::NodeRef>::type NodeType;
>
> unsigned N = 0;
> bool MultipleRoots = (DT.Roots.size() > 1);
> if (MultipleRoots) {
> - typename DominatorTreeBase<typename GraphT::NodeType>::InfoRec &BBInfo =
> - DT.Info[nullptr];
> + auto &BBInfo = DT.Info[nullptr];
> BBInfo.DFSNum = BBInfo.Semi = ++N;
> BBInfo.Label = nullptr;
>
> @@ -188,14 +187,13 @@ void Calculate(DominatorTreeBase<typenam
> Buckets[i] = i;
>
> for (unsigned i = N; i >= 2; --i) {
> - typename GraphT::NodeType* W = DT.Vertex[i];
> - typename DominatorTreeBase<typename GraphT::NodeType>::InfoRec &WInfo =
> - DT.Info[W];
> + typename GraphT::NodeRef W = DT.Vertex[i];
> + auto &WInfo = DT.Info[W];
>
> // Step #2: Implicitly define the immediate dominator of vertices
> for (unsigned j = i; Buckets[j] != i; j = Buckets[j]) {
> - typename GraphT::NodeType* V = DT.Vertex[Buckets[j]];
> - typename GraphT::NodeType* U = Eval<GraphT>(DT, V, i + 1);
> + typename GraphT::NodeRef V = DT.Vertex[Buckets[j]];
> + typename GraphT::NodeRef U = Eval<GraphT>(DT, V, i + 1);
> DT.IDoms[V] = DT.Info[U].Semi < i ? U : W;
> }
>
> @@ -207,7 +205,7 @@ void Calculate(DominatorTreeBase<typenam
> for (typename InvTraits::ChildIteratorType CI =
> InvTraits::child_begin(W),
> E = InvTraits::child_end(W); CI != E; ++CI) {
> - typename InvTraits::NodeType *N = *CI;
> + typename InvTraits::NodeRef N = *CI;
> if (DT.Info.count(N)) { // Only if this predecessor is reachable!
> unsigned SemiU = DT.Info[Eval<GraphT>(DT, N, i + 1)].Semi;
> if (SemiU < WInfo.Semi)
> @@ -227,17 +225,17 @@ void Calculate(DominatorTreeBase<typenam
> }
>
> if (N >= 1) {
> - typename GraphT::NodeType* Root = DT.Vertex[1];
> + typename GraphT::NodeRef Root = DT.Vertex[1];
> for (unsigned j = 1; Buckets[j] != 1; j = Buckets[j]) {
> - typename GraphT::NodeType* V = DT.Vertex[Buckets[j]];
> + typename GraphT::NodeRef V = DT.Vertex[Buckets[j]];
> DT.IDoms[V] = Root;
> }
> }
>
> // Step #4: Explicitly define the immediate dominator of each vertex
> for (unsigned i = 2; i <= N; ++i) {
> - typename GraphT::NodeType* W = DT.Vertex[i];
> - typename GraphT::NodeType*& WIDom = DT.IDoms[W];
> + typename GraphT::NodeRef W = DT.Vertex[i];
> + typename GraphT::NodeRef &WIDom = DT.IDoms[W];
> if (WIDom != DT.Vertex[DT.Info[W].Semi])
> WIDom = DT.IDoms[WIDom];
> }
> @@ -248,34 +246,32 @@ void Calculate(DominatorTreeBase<typenam
> // one exit block, or it may be the virtual exit (denoted by (BasicBlock *)0)
> // which postdominates all real exits if there are multiple exit blocks, or
> // an infinite loop.
> - typename GraphT::NodeType* Root = !MultipleRoots ? DT.Roots[0] : nullptr;
> + typename GraphT::NodeRef Root = !MultipleRoots ? DT.Roots[0] : nullptr;
>
> DT.RootNode =
> (DT.DomTreeNodes[Root] =
> - llvm::make_unique<DomTreeNodeBase<typename GraphT::NodeType>>(
> - Root, nullptr)).get();
> + llvm::make_unique<DomTreeNodeBase<NodeType>>(Root, nullptr))
> + .get();
>
> // Loop over all of the reachable blocks in the function...
> for (unsigned i = 2; i <= N; ++i) {
> - typename GraphT::NodeType* W = DT.Vertex[i];
> + typename GraphT::NodeRef W = DT.Vertex[i];
>
> // Don't replace this with 'count', the insertion side effect is important
> if (DT.DomTreeNodes[W])
> continue; // Haven't calculated this node yet?
>
> - typename GraphT::NodeType* ImmDom = DT.getIDom(W);
> + typename GraphT::NodeRef ImmDom = DT.getIDom(W);
>
> assert(ImmDom || DT.DomTreeNodes[nullptr]);
>
> // Get or calculate the node for the immediate dominator
> - DomTreeNodeBase<typename GraphT::NodeType> *IDomNode =
> - DT.getNodeForBlock(ImmDom);
> + DomTreeNodeBase<NodeType> *IDomNode = DT.getNodeForBlock(ImmDom);
>
> // Add a new tree node for this BasicBlock, and link it as a child of
> // IDomNode
> DT.DomTreeNodes[W] = IDomNode->addChild(
> - llvm::make_unique<DomTreeNodeBase<typename GraphT::NodeType>>(
> - W, IDomNode));
> + llvm::make_unique<DomTreeNodeBase<NodeType>>(W, IDomNode));
> }
>
> // Free temporary memory used to construct idom's
>
> Modified: llvm/trunk/lib/IR/Dominators.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Dominators.cpp?rev=278961&r1=278960&r2=278961&view=diff
> ==============================================================================
> --- llvm/trunk/lib/IR/Dominators.cpp (original)
> +++ llvm/trunk/lib/IR/Dominators.cpp Wed Aug 17 15:01:58 2016
> @@ -64,9 +64,13 @@ template class llvm::DomTreeNodeBase<Bas
> template class llvm::DominatorTreeBase<BasicBlock>;
>
> template void llvm::Calculate<Function, BasicBlock *>(
> - DominatorTreeBase<GraphTraits<BasicBlock *>::NodeType> &DT, Function &F);
> + DominatorTreeBase<
> + typename std::remove_pointer<GraphTraits<BasicBlock *>::NodeRef>::type>
> + &DT,
> + Function &F);
> template void llvm::Calculate<Function, Inverse<BasicBlock *>>(
> - DominatorTreeBase<GraphTraits<Inverse<BasicBlock *>>::NodeType> &DT,
> + DominatorTreeBase<typename std::remove_pointer<
> + GraphTraits<Inverse<BasicBlock *>>::NodeRef>::type> &DT,
> Function &F);
>
> // dominates - Return true if Def dominates a use in User. This performs
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list