[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};
----------------
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.


https://reviews.llvm.org/D47893