[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

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-varargs.ll

 ; RUN: opt < %s -inline -S | FileCheck %s
 ; RUN: opt < %s -passes='cgscc(inline,function(instcombine))' -S | FileCheck %s
 
 declare void @ext_method(i8*, i32)
 declare signext i16 @vararg_fn(...) #0
 declare "cc 9" void @vararg_fn_cc9(i8* %p, ...)
+%struct.__va_list = type { i8* }
 
 define linkonce_odr void @thunk(i8* %this, ...) {
   %this_adj = getelementptr i8, i8* %this, i32 4
   musttail call void (i8*, ...) bitcast (void (i8*, i32)* @ext_method to void (i8*, ...)*)(i8* nonnull %this_adj, ...)
   ret void
 }
 
 define void @thunk_caller(i8* %p) {
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, ...) local_unnamed_addr #0 {
+entry:
+  %ap = alloca %struct.__va_list, align 4
+  %ap2 = alloca %struct.__va_list, align 4
+  %0 = bitcast %struct.__va_list* %ap to i8*
+  %1 = bitcast %struct.__va_list* %ap to i8*
+  call void @llvm.va_start(i8* nonnull %0)
+  call fastcc void @callee_with_vaend(%struct.__va_list* nonnull %ap)
+  call void @llvm.va_start(i8* nonnull %1)
+  call fastcc void @callee_with_vaend_alwaysinline(%struct.__va_list* nonnull %ap2)
+  ret void
+}
+
+define internal fastcc void @callee_with_vaend_alwaysinline(%struct.__va_list* %a) unnamed_addr alwaysinline {
+entry:
+  %0 = bitcast %struct.__va_list* %a to i8*
+  tail call void @llvm.va_end(i8* %0)
+  ret void
+}
+
+define internal fastcc void @callee_with_vaend(%struct.__va_list* %a) unnamed_addr {
+entry:
+  %0 = bitcast %struct.__va_list* %a to i8*
+  tail call void @llvm.va_end(i8* %0)
+  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" }