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

Eli Friedman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Aug 8 11:40:58 PDT 2018


efriedma added inline comments.


================
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()
----------------
labrinea wrote:
> efriedma wrote:
> > 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?
> > 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?
> We create an empty CHI-node with the same Value Number for each instruction in V. Then in fillChiArgs() we fill the CHI-nodes using the Rename Stack, while walking the post-dominator tree top-down. Using ubsan I realized we create a huge amount of CHIs that correspond to the same <VN,Instr> pair for a given IDFB.
> 
> > 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?
> I think not. A given instruction has always the same Value Number. @sebpop do you have better insight on this? Maybe we could use something like this to be more assertive but I believe it's unnecessary:
> `using CHICache = DenseMap<BasicBlock *, SmallSet<std::pair<VNType, Instruction *>, 2>>;`
> (or a DenseSet if preferable)
Okay, that makes sense.


https://reviews.llvm.org/D50323





More information about the llvm-commits mailing list