[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

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

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

64

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

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

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

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

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

[sabuasal]

comments addressed in committed version rL342675.

Thanks for review!

lib/Analysis/InlineCost.cpp

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;
         return false;
       }
     }
 
     if (F == CS.getInstruction()->getFunction()) {
       // This flag will fully abort the analysis, so don't bother with anything
       // else.
       IsRecursiveCall = true;
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.
         case llvm::Intrinsic::vastart:
-        case llvm::Intrinsic::vaend:
           return false;
         }
     }
   }
 
   return true;
 }
 

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

+; RUN: opt < %s -inline -S | FileCheck %s
+
+; This test checks that we inline functions that have va_end, if
+; they dob't have va_start.
+
+%struct.__va_list = type { i8* }
+
+; Function Attrs: noinline nounwind optnone
+define dso_local void @caller(i32 %n, i8* noalias %args, ...) {
+entry:
+  %n.addr = alloca i32, align 4
+  %args.addr = alloca i8*, align 4
+  %ap = alloca %struct.__va_list, align 4
+  store i32 %n, i32* %n.addr, align 4
+  store i8* %args, i8** %args.addr, align 4
+  %ap1 = bitcast %struct.__va_list* %ap to i8*
+  call void @llvm.va_start(i8* %ap1)
+  %0 = load i32, i32* %n.addr, align 4
+  %coerce.dive = getelementptr inbounds %struct.__va_list, %struct.__va_list* %ap, i32 0, i32 0
+  %1 = bitcast i8** %coerce.dive to [1 x i32]*
+  %2 = load [1 x i32], [1 x i32]* %1, align 4
+  call void @always_inlined(i32 %0, [1 x i32] %2)
+  %ap2 = bitcast %struct.__va_list* %ap to i8*
+  call void @llvm.va_end(i8* %ap2)
+  ret void
+}
+
+; CHECK-LABEL: @caller
+; CHECK-NOT:   call void @always_inlined
+
+
+; Function Attrs: nounwind
+declare void @llvm.va_start(i8*)
+
+; Function Attrs: alwaysinline nounwind
+define void @always_inlined(i32 %n, [1 x i32] %a.coerce) {
+entry:
+  %a = alloca %struct.__va_list, align 4
+  %n.addr = alloca i32, align 4
+  %a_copy = alloca %struct.__va_list, align 4
+  %coerce.dive = getelementptr inbounds %struct.__va_list, %struct.__va_list* %a, i32 0, i32 0
+  %0 = bitcast i8** %coerce.dive to [1 x i32]*
+  store [1 x i32] %a.coerce, [1 x i32]* %0, align 4
+  store i32 %n, i32* %n.addr, align 4
+  %1 = bitcast %struct.__va_list* %a_copy to i8*
+  %2 = bitcast %struct.__va_list* %a to i8*
+  call void @llvm.va_copy(i8* %1, i8* %2)
+  %3 = load i32, i32* %n.addr, align 4
+  %coerce.dive1 = getelementptr inbounds %struct.__va_list, %struct.__va_list* %a_copy, i32 0, i32 0
+  %4 = bitcast i8** %coerce.dive1 to [1 x i32]*
+  %5 = load [1 x i32], [1 x i32]* %4, align 4
+  call void @foo([1 x i32] %5, i32 %3)
+  %a_copy2 = bitcast %struct.__va_list* %a_copy to i8*
+  call void @llvm.va_end(i8* %a_copy2)
+  ret void
+}
+
+; Function Attrs: nounwind
+declare void @llvm.va_end(i8*)
+
+; Function Attrs: nounwind
+declare void @llvm.va_copy(i8*, i8*)
+
+declare dso_local void @foo([1 x i32], i32)