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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

Sorry, I think you should have argued with one of my suggestions, see below. =D Your code was already right.

I also have one comment below that I'd mostly like to talk about and address in a follow-up patch if needed, it doesn't need to block this going in. Feel free to submit once you go back to your original (better) approach on the loop.



================
Comment at: lib/Analysis/InlineCost.cpp:52-62
+static cl::opt<int>
+    ColdCallSiteThreshold("inline-cold-callsite-threshold", cl::Hidden,
+                          cl::init(45),
+                          cl::desc("Threshold for inlining cold callsites"));
+
 // We introduce this threshold to help performance of instrumentation based
 // PGO before we actually hook up inliner with analysis passes such as BPI and
----------------
Is there any way to avoid introducing yet another tunable here? It feels weird to have two cold tunables, and more weird for them to be different.

Also, I would expect the threshold here to be *very* similar to the -Oz inlining threshold. Should they be the same? Or at least documented together?


================
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)
----------------
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.


https://reviews.llvm.org/D28331