[PATCH] D50323: [GVNHoist] Prune out useless CHI insertions

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a reviewer: hiraditya.
efriedma added inline comments.


================
Comment at: lib/Transforms/Scalar/GVNHoist.cpp:158
 using CHIArgs = iterator_range<CHIIt>;
+using CHICache = DenseMap<BasicBlock *, SmallPtrSet<Instruction *, 2>>;
 using OutValuesType = DenseMap<BasicBlock *, SmallVector<CHIArg, 2>>;
----------------
`DenseSet<std::pair<BasicBlock*, Instruction*>>` is probably more efficient if you expect a small number of instructions per BB.


================
Comment at: lib/Transforms/Scalar/GVNHoist.cpp:792
       // Make a map of BB vs instructions to be hoisted.
       for (unsigned i = 0; i < V.size(); ++i) {
         InValue[V[i]->getParent()].push_back(std::make_pair(VN, V[i]));
----------------
labrinea wrote:
> xbolva00 wrote:
> > xbolva00 wrote:
> > > unsigned i = 0, e = V.size(); i < e; ++i
> > > 
> > > 
> > > same below
> > or better if possible.. for (auto &I : V) ?
> Seems a nit and irrelevant to my changes, but I could do that.
(If you do decide to fix this, please do it in a separate commit.)


================
Comment at: lib/Transforms/Scalar/GVNHoist.cpp:803
+              CachedCHIs[IDFB].insert(V[i]).second) {
             OutValue[IDFB].push_back(C);
             LLVM_DEBUG(dbgs() << "\nInsertion a CHI for BB: " << IDFB->getName()
----------------
If I'm following the code correctly, we push `V.size()` copies of C into `OutValue[IDFB]`?  That seems weird; can you explain why we do that?

CachedCHIs is a set of instructions, but OutValue is a map of CHIArgs.  A given instruction can have multiple value numbers, right?  What's the impact of only allowing one of those numbers to be inserted in OutValue?


https://reviews.llvm.org/D50323