[PATCH] D28331: Improve PGO support for the new inliner

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1408-1409
+  SmallPtrSet<BasicBlock *, 16> ClonedBBs;
+  for (auto &OrigBB : *Callee) {
+    BasicBlock *ClonedBB = cast_or_null<BasicBlock>(VMap[&OrigBB]);
+    if (!ClonedBB)
----------------
eraman wrote:
> chandlerc wrote:
> > I was hoping we could do still better by walking the inlined region in the caller so we would *only* touch cloned blocks. However, there isn't a map from cloned BB to original BB, which is why you can't do that. And why you wrote the code you did. Sorry for the confusion.
> > 
> > Unfortunately, I think walking the entire map is better than walking the entire callee as the map only contains the inlined part. Also, in all of the other code here we walk the map to find relatively sparse and infrequent things so it seems fine. Like I said, sorry for sending you down a wrong path, I hadn't thought about the lack of clone -> original BB.
> But the size of the value map is O(|instructions in the cloned region|) and not O(|BBs in the cloned region|). My thinking was O(|instructions in the cloned region|) likely to be greater than O(|BBs in the original callee|) and that's why you preferred this. 
Well, we already do tons of things O(|instructions in the cloned region|) including walking the VMap in at least three other places and the original InlineCost computation. This is typically OK because we have a constant bound on it (the threshold).

But we have no realistic bound on the number of basic blocks in the callee. So it might be faster on average but have much worse long tail.

I think I'd feel safer staying within the inlined region whether its instructions or basic blocks. And if this proves hot, we should just add a reverse map (at least for basic blocks) and walk the basic blocks in the cloned region.


https://reviews.llvm.org/D28331