This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by sabuasal on Sep 13 2018, 6:44 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

sabuasal created this revision.Sep 13 2018, 6:44 PM
fhahn added inline comments.Sep 14 2018, 2:06 AM
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 updated this revision to Diff 166212.Sep 19 2018, 7:12 PM
sabuasal marked 2 inline comments as done.
  • Simplified the test case.
  • Added it to inline-varargs instead of having it in a separate file.
sabuasal marked an inline comment as done.Sep 19 2018, 7:14 PM
sabuasal added inline comments.
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 added a comment.Sep 20 2018, 5:07 AM

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 accepted this revision.Sep 20 2018, 5:10 AM

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

This revision is now accepted and ready to land.Sep 20 2018, 5:10 AM
This revision was automatically updated to reflect the committed changes.
sabuasal marked 3 inline comments as done.

comments addressed in committed version rL342675.

Thanks for review!