[PartialInlining] Use isInlineViable to detect constructs preventing inlining.

[fhahn]

Summary

Use isInlineViable to prevent inlining of functions with non-inlinable
constructs, in case cost analysis is skipped.

[fhahn]

ping

[efriedma]

isInlineViable doesn't care whether the code is actually reachable, so you could pessimize partial inlining in certain cases, e.g. a function which calls va_start conditionally. I don't think we need fix that now, but it would be nice to at least have a testcase demonstrating the issue.

[fhahn]

In D42846#1005429, @efriedma wrote:

isInlineViable doesn't care whether the code is actually reachable, so you could pessimize partial inlining in certain cases, e.g. a function which calls va_start conditionally. I don't think we need fix that now, but it would be nice to at least have a testcase demonstrating the issue.

Thanks Eli, I will try to come up with a test case tomorrow.

[fhahn]

I could not find a case where not considering reachability would prevent us from partial inlining with this patch.

Currently, CodeExtractor won't extract unreachable blocks (via DT), blocks with vastart/vaend or blockaddresses in the function we use for partial inlining. That only leaves localescape which isInlineViable prevents, and those calls always have to be in the entry block, so they are always reachable. I think it would be better to avoid extracting blocks with localesacpe calls for now and just assert isInlineViable.

If we teach CodeExtractor to consider reachability beyond using the dominator tree, we should consider the same reachability in isInlineViable.

Independent of this patch, considering reachability in isInlineViable could be beneficial for alwaysinline functions, but that should be an independent patch I think. What do you think @efriedma , does this make sense?

[efriedma]

Can you explain the difference between llvm::isInlineViable and CodeExtractor::isBlockValidForExtraction? I'm kind of confused why they're checking different things.

[fhahn]

In D42846#1008060, @efriedma wrote:

Can you explain the difference between llvm::isInlineViable and CodeExtractor::isBlockValidForExtraction? I'm kind of confused why they're checking different things.

I think CodeExtractor is not only used to extract code for partial inlining, but also used by LoopExtractor. I suppose this is why they do not check the same thing, but I think it would make sense to check for non-inlinable blocks in CodeExtractor too. Maybe we should add an isInlineableBlock function and use them in isBlockValidForExtraction?

[efriedma]

It would be good to clean them up to make it clear exactly how they differ, yes.

[fhahn]

Update to use isInlineViable only when cost analysis is skipped.

I think CodeExtractor is not only used to extract code for partial inlining, but also used by LoopExtractor. I suppose this is why they do not check the same thing, but I think it would make sense to check for non-inlinable blocks in CodeExtractor too. Maybe we should add an isInlineableBlock function and use them in isBlockValidForExtraction?

After having another look, the difference between isBlockValidForExtraction and isInlineViable is that isBlockValidForExtraction checks loops that are going to be outlined, rather than inlined.

[fhahn]

ping

[efriedma]

LGTM

llvm/trunk/lib/Transforms/IPO/PartialInlining.cpp

PartialInlinerImpl::getOutliningCallBBRelativeFreq(FunctionCloner &Cloner) {
   return OutlineRegionRelFreq;
 }
 
 bool PartialInlinerImpl::shouldPartialInline(
     CallSite CS, FunctionCloner &Cloner,
     BlockFrequency WeightedOutliningRcost) {
   using namespace ore;
 
-  if (SkipCostAnalysis)
-    return true;
-
   Instruction *Call = CS.getInstruction();
   Function *Callee = CS.getCalledFunction();
   assert(Callee == Cloner.ClonedFunc);
 
+  if (SkipCostAnalysis)
+    return isInlineViable(*Callee);
+
   Function *Caller = CS.getCaller();
   auto &CalleeTTI = (*GetTTI)(*Callee);
   auto &ORE = (*GetORE)(*Caller);
   InlineCost IC = getInlineCost(CS, getInlineParams(), CalleeTTI,
                                 *GetAssumptionCache, GetBFI, PSI, &ORE);
 
   if (IC.isAlways()) {
     ORE.emit([&]() {

llvm/trunk/test/Transforms/CodeExtractor/PartialInlineNotViable.ll

+; RUN: opt < %s -partial-inliner -skip-partial-inlining-cost-analysis -S | FileCheck %s
+;
+
+define i32 @callee_indr_branch(i32 %v) {
+entry:
+  %cmp = icmp sgt i32 %v, 2000
+  %addr = select i1 %cmp, i8* blockaddress(@callee_indr_branch, %if.then), i8* blockaddress(@callee_indr_branch, %if.end)
+  indirectbr i8* %addr, [ label %if.then, label %if.end]
+
+if.then:                                          ; preds = %entry
+  %mul = mul nsw i32 %v, 10
+  br label %if.then2
+
+if.then2:
+  %sub = sub i32 %v, 10
+  br label %if.end
+
+if.end:                                           ; preds = %if.then, %entry
+  %v2 = phi i32 [ %v, %entry ], [ %mul, %if.then2 ]
+  %add = add nsw i32 %v2, 200
+  ret i32 %add
+}
+
+declare void @use_fp(i8 *)
+declare void @llvm.localescape(...)
+declare i8* @llvm.frameaddress(i32)
+declare i8* @llvm.localrecover(i8*, i8*, i32)
+
+
+
+define i32 @callee_frameescape(i32 %v) {
+entry:
+  %a = alloca i32
+  call void (...) @llvm.localescape(i32* %a)
+  %cmp = icmp sgt i32 %v, 2000
+  br i1 %cmp, label %if.then, label %if.end
+
+if.then:                                          ; preds = %entry
+  %mul = mul nsw i32 %v, 10
+  br label %if.then2
+
+if.then2:
+  %sub = sub i32 %v, 10
+  br label %if.end
+
+if.end:                                           ; preds = %if.then, %entry
+  %v2 = phi i32 [ %v, %entry ], [ %mul, %if.then2 ]
+  %add = add nsw i32 %v2, 200
+  ret i32 %add
+}
+
+
+; CHECK-LABEL: @caller
+; CHECK: %r1 = call i32 @callee_indr_branch(i32 %v)
+; CHECK-NEXT: %r2 = call i32 @callee_frameescape(i32 %v)
+define i32 @caller(i32 %v) {
+entry:
+  %r1 = call i32 @callee_indr_branch(i32 %v)
+  %r2 = call i32 @callee_frameescape(i32 %v)
+  %res = add i32 %r1, %r2
+  ret i32 %res
+}
+