[PATCH] D47893: Add a PhiValuesAnalysis pass to calculate the underlying values of phis

[John Brawn via Phabricator via llvm-commits]

john.brawn added inline comments.


================
Comment at: lib/Analysis/PhiValues.cpp:45
+// on B they need to depend on the phis that B depends on.
+void PhiValues::processPhi(const PHINode *InitialPhi) {
+  SmallVector<const PHINode *, 4> Worklist = {InitialPhi};
----------------
john.brawn wrote:
> sebpop wrote:
> > Why not using Danny's implementation of Tarjan's algorithm?
> > processPhi() would be findSCC()
> > 
> > >>! In D44564#1040222, @dberlin wrote:
> > > https://reviews.llvm.org/D28934 has a version of the SCC finder you can
> > > borrow if you want (TarjanSCC class)
> > 
> > ```
> > // Tarjan's algorithm with Nuutila's improvements
> > // Wish we could use SCCIterator, but graph traits makes it *very* hard to
> > // create induced subgraphs because it
> > // 1. only works on pointers, so i can't just create an intermediate struct
> > // 2. the functions are static, so i can't just override them in a subclass of
> > // graphtraits, or otherwise store state in the struct.
> > 
> > struct TarjanSCC {
> >   unsigned int DFSNum = 1;
> >   SmallPtrSet<Value *, 8> InComponent;
> >   DenseMap<Value *, unsigned int> Root;
> >   SmallPtrSet<Value *, 8> PHISet;
> >   SmallVector<Value *, 8> Stack;
> >   // Store the components as vector of ptr sets, because we need the topo order
> >   // of SCC's, but not individual member order
> >   SmallVector<SmallPtrSet<PHINode *, 8>, 8> Components;
> >   TarjanSCC(const SmallVectorImpl<PHINode *> &Phis)
> >       : PHISet(Phis.begin(), Phis.end()) {
> >     for (auto Phi : Phis)
> >       if (Root.lookup(Phi) == 0)
> >         FindSCC(Phi);
> >   }
> >   void FindSCC(PHINode *Phi) {
> >     Root[Phi] = ++DFSNum;
> >     // Store the DFS Number we had before it possibly gets incremented.
> >     unsigned int OurDFS = DFSNum;
> >     InComponent.erase(Phi);
> >     for (auto &PhiOp : Phi->operands()) {
> >       // Only inducing the subgraph of phis we got passed
> >       if (!PHISet.count(PhiOp))
> >         continue;
> >       if (Root.lookup(PhiOp) == 0)
> >         FindSCC(cast<PHINode>(PhiOp));
> >       if (!InComponent.count(PhiOp))
> >         Root[Phi] = std::min(Root.lookup(Phi), Root.lookup(PhiOp));
> >     }
> >     // See if we really were the root, by seeing if we still have our DFSNumber,
> >     // or something lower
> >     if (Root.lookup(Phi) == OurDFS) {
> >       Components.resize(Components.size() + 1);
> >       auto &Component = Components.back();
> >       Component.insert(Phi);
> >       DEBUG(dbgs() << "Component root is " << *Phi << "\n");
> >       InComponent.insert(Phi);
> >       while (!Stack.empty() && Root.lookup(Stack.back()) >= OurDFS) {
> >         DEBUG(dbgs() << "Component member is " << *Stack.back() << "\n");
> >         Component.insert(cast<PHINode>(Stack.back()));
> >         InComponent.insert(Stack.back());
> >         Stack.pop_back();
> >       }
> >     } else {
> >       // Part of a component, push to stack
> >       Stack.push_back(Phi);
> >     }
> >   }
> > };
> > ```
> > Why not using Danny's implementation of Tarjan's algorithm?
> > processPhi() would be findSCC()
> 
> The two are doing two different things:
>  * findSCC partitions the phi graph into a set of subgraphs where each subgraph is an SCC (each node has an edge to every other node)
>  * processPhi (implicitly) partitions the phi graph into a set of subgraphs where the underlying values reachable in a subgraph is the same
> 
> Taking the long_phi_chain.ll test as an example, findSCC gives us the components:
>  # { phi1, phi2 }
>  # { phi3, phi4, phi5 }
>  # { phi6, phi7 }
>  # { phi8, phi9, phi10 }
>  # { phi11, phi12, phi13 }
>  # { phi14 }
>  # { phi15 }
> 
> which then form an SCC graph 7 -> 6 -> 5 -> 4 -> 3 -> 2 -> 1.
> 
> processPhi however needs to find that all of these phis have the same set of underlying values, and I don't see a way to do this other than looking through the SCC graph in a way very similar to how processPhi looks through the phi graph (though the SCC graph will be simpler than the phi graph).
> 
> It may be possible though to do something similar to findSCC in order to explicitly construct a 'set of phis that have the same underlying values' partitioning, i.e. numbering nodes and building up stack then stacked nodes with the same number are in the same set, but I don't know if it will be simpler/faster than what I have here. I'll give it a try though.
> It may be possible though to do something similar to findSCC in order to explicitly construct a 'set of phis that have the same underlying values' partitioning, i.e. numbering nodes and building up stack then stacked nodes with the same number are in the same set, but I don't know if it will be simpler/faster than what I have here. I'll give it a try though.

Because Tarjan's algorithm completes SSCs in bottom-up order, and the SCC graph is guaranteed to have no cycles, this turns out to be fairly straighforward. I'll rework this to do that.


https://reviews.llvm.org/D47893