[PATCH] D145516: [Inliner] Avoid excessive inlining through devirtualised calls

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

apologies, was out on vacation for a while



================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:974-975
+                  CalleeSCC != CG.lookupSCC(CG.get(*NewCallee)))  {
+                // This newly devirtualised function call might form a cycle in
+                // our RefSCC, don't repeatedly inline it as mentioned above.
+                // Take the cost multiplier from the new call-site function:
----------------
IIUC, there's never actually a cycle (ref or call) in the IR. The problem is that after inlining, we do see the same sort of issue as with inlining through a child SCC because the indirect calls end up ping ponging between `@longname` and `@rec_call_to_longname`.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:976-980
+                // Take the cost multiplier from the new call-site function:
+                // we do not get the SCC-post-order traversal guarantee as
+                // NewCallee is in a different SCC, thus a different SCCs
+                // inlining NewCallee might give it different multipliers each
+                // time.
----------------
I don't understand this comment. I tried the following diff and it adds the multiplier attributes as expected
```
@@ -418,8 +420,15 @@ PreservedAnalyses InlinerPass::run(LazyCallGraph::SCC &InitialC,
               // inline through the SCC the function will end up being
               // self-recursive which the inliner bails out on, and inlining
               // within an SCC is necessary for performance.
-              if (CalleeSCC != C &&
-                  CalleeSCC == CG.lookupSCC(CG.get(*NewCallee))) {
+              auto WasCallEdge = [&CG](Function &Callee, Function &NewCallee) {
+                LazyCallGraph::Node &CalleeN = CG.get(Callee);
+                LazyCallGraph::Node &NewCalleeN = CG.get(NewCallee);
+                LazyCallGraph::Edge *E = CalleeN->lookup(NewCalleeN);
+                return E && E->isCall();
+              };
+              if ((CalleeSCC != C &&
+                   CalleeSCC == CG.lookupSCC(CG.get(*NewCallee))) ||
+                  !WasCallEdge(Callee, *NewCallee)) {
                 Attribute NewCBCostMult = Attribute::get(
                     M.getContext(),
                     InlineConstants::FunctionInlineCostMultiplierAttributeName,
```


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:989
+                    InlineConstants::FunctionInlineCostMultiplierAttributeName,
+                    itostr(std::min(CBCostMult * IntraSCCCostMultiplier, 10000)));
+                NewCallee->addFnAttr(NewCBCostMult);
----------------
what's the 10000 for, can we reuse the existing code?


================
Comment at: llvm/test/Transforms/Inline/devirtualize-7.ll:1
+; RUN: opt %s -o - --passes='cgscc(inline)' -S | FileCheck %s --implicit-check-not=@extern2
+
----------------
slightly reduced test case
```
target triple = "x86_64-unknown-unknown"

declare void @extern1()

define internal void @rec_call_to_longname(ptr %longname) {
  call void %longname(ptr @rec_call_to_longname)
  ret void
}

define internal void @longname(ptr %0) {
  call void @extern1()
  call void @extern1()
  call void @extern1()
  call void %0(ptr @longname)  ; Exponential growth occurs between these two devirt calls.
  call void %0(ptr @longname)
  ret void
}

define void @e() {
  call void @c()
  call void @c()
  ret void
}

define internal void @c() {
  call void @b()
  ret void
}

define internal void @b() {
  call void @a()
  ret void
}

define internal void @a() {
  call void @rec_call_to_longname(ptr @longname)
  ret void
}
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145516/new/

https://reviews.llvm.org/D145516