[inline Cost] Don't mark function accessing varargs as non-inlinable

[sabuasal]

Summary

rL323619 upstream marks functions that are calling va_end as not viable for
inlining. This patch reverses that since this va_end doesn't need
access to the vriadic arguments list that are saved on the stack, only
va_start does.

[fhahn]

test/Transforms/Inline/inline-func-vaend.ll

1 ↗

(On Diff #165406)

Would it be possible to simplify the test case? I don't think we need most of the loads, stores and the %struct.__va_list type. Also I think it would be good to move the test case to the other vararg related test (inline-varargs.ll IIRC)

35 ↗

(On Diff #165406)

It looks like the alwaysinline attribute got dropped? I think it would be good to test both with and without the attribute.

64 ↗

(On Diff #165406)

do we need foo?

[sabuasal]

• Simplified the test case.
• Added it to inline-varargs instead of having it in a separate file.

[sabuasal]

test/Transforms/Inline/inline-func-vaend.ll

1 ↗

(On Diff #165406)

I simplified it. I am not sure how I can get rid of va_list; what i am testing is va_start calledin the caller and va_end in the callee. I need to have va_list to pass it from the caller to the callee.

Any suggestions?

35 ↗

(On Diff #165406)

added tests with and without alwaysinline.

[fhahn]

I simplified it. I am not sure how I can get rid of va_list; what i am testing is va_start calledin the caller and va_end in the callee. I need to have va_list to pass it from the caller to the callee.

IIUC you can just pass the arglist pointer as i8*, like below

+define void @caller_with_vastart(i8* %args, ...) {
+entry:
+  %ap = alloca i8*, align 4
+  %ap.ptr = bitcast i8** %ap to i8*
+  %ap2 = alloca i8*, align 4
+  %ap2.ptr = bitcast i8** %ap2 to i8*
+  call void @llvm.va_start(i8* nonnull %ap.ptr)
+  call fastcc void @callee_with_vaend(i8* nonnull %ap.ptr)
+  call void @llvm.va_start(i8* nonnull %ap2.ptr)
+  call fastcc void @callee_with_vaend_alwaysinline(i8* nonnull %ap2.ptr)
+  ret void
+}
+
+define internal fastcc void @callee_with_vaend_alwaysinline(i8* %a) alwaysinline {
+entry:
+  tail call void @llvm.va_end(i8* %a)
+  ret void
+}
+
+define internal fastcc void @callee_with_vaend(i8* %a) {
+entry:
+  tail call void @llvm.va_end(i8* %a)
+  ret void
+}

lib/Analysis/InlineCost.cpp

2081 ↗

(On Diff #166212)

This needs changing I think to 'Disallow inlining of functions that initialize a vararg list' or something.

[fhahn]

LGTM, with my previous comments.

lib/Analysis/InlineCost.cpp

1242 ↗

(On Diff #166212)

Maybe it would make sense to change this to InitsVarArgs or something now

[sabuasal]

comments addressed in committed version rL342675.

Thanks for review!

llvm/trunk/lib/Analysis/InlineCost.cpp

class CallAnalyzer : public InstVisitor<CallAnalyzer, bool> {
   bool IsCallerRecursive;
   bool IsRecursiveCall;
   bool ExposesReturnsTwice;
   bool HasDynamicAlloca;
   bool ContainsNoDuplicateCall;
   bool HasReturn;
   bool HasIndirectBr;
   bool HasUninlineableIntrinsic;
-  bool UsesVarArgs;
+  bool InitsVargArgs;
 
   /// Number of bytes allocated statically by the callee.
   uint64_t AllocatedSize;
   unsigned NumInstructions, NumVectorInstructions;
   int VectorBonus, TenPercentVectorBonus;
   // Bonus to be applied when the callee has only one reachable basic block.
   int SingleBBBonus;
 
CallAnalyzer(const TargetTransformInfo &TTI,
       : TTI(TTI), GetAssumptionCache(GetAssumptionCache), GetBFI(GetBFI),
         PSI(PSI), F(Callee), DL(F.getParent()->getDataLayout()), ORE(ORE),
         CandidateCS(CSArg), Params(Params), Threshold(Params.DefaultThreshold),
         Cost(0), ComputeFullInlineCost(OptComputeFullInlineCost ||
                                        Params.ComputeFullInlineCost || ORE),
         IsCallerRecursive(false), IsRecursiveCall(false),
         ExposesReturnsTwice(false), HasDynamicAlloca(false),
         ContainsNoDuplicateCall(false), HasReturn(false), HasIndirectBr(false),
-        HasUninlineableIntrinsic(false), UsesVarArgs(false), AllocatedSize(0),
+        HasUninlineableIntrinsic(false), InitsVargArgs(false), AllocatedSize(0),
         NumInstructions(0), NumVectorInstructions(0), VectorBonus(0),
         SingleBBBonus(0), EnableLoadElimination(true), LoadEliminationCost(0),
         NumConstantArgs(0), NumConstantOffsetPtrArgs(0), NumAllocaArgs(0),
         NumConstantPtrCmps(0), NumConstantPtrDiffs(0),
         NumInstructionsSimplified(0), SROACostSavings(0),
         SROACostSavingsLost(0) {}
 
   InlineResult analyzeCall(CallSite CS);
if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(CS.getInstruction())) {
         disableLoadElimination();
         // SROA can usually chew through these intrinsics, but they aren't free.
         return false;
       case Intrinsic::icall_branch_funnel:
       case Intrinsic::localescape:
         HasUninlineableIntrinsic = true;
         return false;
       case Intrinsic::vastart:
-      case Intrinsic::vaend:
-        UsesVarArgs = true;
+        InitsVargArgs = true;
         return false;
       }
     }
 
     if (F == CS.getInstruction()->getFunction()) {
       // This flag will fully abort the analysis, so don't bother with anything
       // else.
       IsRecursiveCall = true;
for (BasicBlock::iterator I = BB->begin(), E = BB->end(); I != E; ++I) {
     else if (ExposesReturnsTwice)
       IR = "exposes returns twice";
     else if (HasDynamicAlloca)
       IR = "dynamic alloca";
     else if (HasIndirectBr)
       IR = "indirect branch";
     else if (HasUninlineableIntrinsic)
       IR = "uninlinable intrinsic";
-    else if (UsesVarArgs)
+    else if (InitsVargArgs)
       IR = "varargs";
     if (!IR) {
       if (ORE)
         ORE->emit([&]() {
           return OptimizationRemarkMissed(DEBUG_TYPE, "NeverInline",
                                           CandidateCS.getInstruction())
                  << NV("Callee", &F) << " has uninlinable pattern ("
                  << NV("InlineResult", IR.message)
for (auto &II : *BI) {
         default:
           break;
         // Disallow inlining of @llvm.icall.branch.funnel because current
         // backend can't separate call targets from call arguments.
         case llvm::Intrinsic::icall_branch_funnel:
         // Disallow inlining functions that call @llvm.localescape. Doing this
         // correctly would require major changes to the inliner.
         case llvm::Intrinsic::localescape:
-        // Disallow inlining of functions that access VarArgs.
+        // Disallow inlining of functions that initialize VarArgs with va_start.
         case llvm::Intrinsic::vastart:
-        case llvm::Intrinsic::vaend:
           return false;
         }
     }
   }
 
   return true;
 }
 

llvm/trunk/test/Transforms/Inline/inline-varargs.ll

define i32 @call_vargs() {
   %res2 = call i32 (...) @varg_accessed_alwaysinline(i32 15)
   %res = add i32 %res1, %res2
   ret i32 %res
 }
 ; CHECK-LABEL: @call_vargs
 ; CHECK: %res1 = call i32 (...) @varg_accessed(i32 10)
 ; CHECK-NEXT: %res2 = call i32 (...) @varg_accessed_alwaysinline(i32 15)
 
+define void @caller_with_vastart(i8* noalias nocapture readnone %args, ...) {
+entry:
+  %ap = alloca i8*, align 4
+  %ap.ptr = bitcast i8** %ap to i8*
+  %ap2 = alloca i8*, align 4
+  %ap2.ptr = bitcast i8** %ap to i8*
+  call void @llvm.va_start(i8* nonnull %ap.ptr)
+  call fastcc void @callee_with_vaend(i8* nonnull %ap.ptr)
+  call void @llvm.va_start(i8* nonnull %ap2.ptr)
+  call fastcc void @callee_with_vaend_alwaysinline(i8* nonnull %ap2.ptr)
+  ret void
+}
+
+define internal fastcc void @callee_with_vaend_alwaysinline(i8* %a) alwaysinline {
+entry:
+  tail call void @llvm.va_end(i8* %a)
+  ret void
+}
+
+define internal fastcc void @callee_with_vaend(i8* %a) {
+entry:
+  tail call void @llvm.va_end(i8* %a)
+  ret void
+}
+
+; CHECK-LABEL: @caller_with_vastart
+; CHECK-NOT: @callee_with_vaend
+; CHECK-NOT: @callee_with_vaend_alwaysinline
+
 declare void @llvm.va_start(i8*)
 declare void @llvm.va_end(i8*)
 
 ; CHECK: attributes [[FN_ATTRS]] = { "foo"="bar" }
 attributes #0 = { "foo"="bar" }