Page MenuHomePhabricator

[PartialInliner] Inline vararg functions that forward varargs.
ClosedPublic

Authored by fhahn on Nov 3 2017, 10:42 AM.

Details

Summary

This patch extends the partial inliner to support inlining parts of
vararg functions, if the vararg handling is done in the outlined part.

It adds a ForwardVarArgsTo argument to InlineFunction. If it is
non-null, all varargs passed to the inlined function will be added to
all calls to ForwardVarArgsTo.

The partial inliner takes care to only pass ForwardVarArgsTo if the
varargs handing is done in the outlined function. It checks that vastart
is not part of the function to be inlined.

test/Transforms/CodeExtractor/PartialInlineNoInline.ll (already part
of the repo) checks we do not do partial inlining if vastart is used in
a basic block that will be inlined.

Diff Detail

Event Timeline

fhahn created this revision.Nov 3 2017, 10:42 AM
fhahn added a subscriber: grosser.Nov 3 2017, 10:43 AM

CC Tobias, as he asked me to look into that in D38585.

fhahn added a subscriber: gyiu.Nov 3 2017, 10:44 AM
grosser requested changes to this revision.Nov 7 2017, 12:58 AM

Hi Florian,

thanks for looking over this patch. Your new variant looks indeed a lot cleaner than my original patch. I like it.

Overall, I think your patch looks good. Just some minor nitpicks and maybe some more documentation for the new arguments you introduce.

Also, AFAIKS the outliner will (temporarily) generate invalid IR as the calls to the outlined function won't forward the vararg parameters. This is only "fixed" after inlining, when the va_start() and va_end() intrinsics again refer to the right function. While I don't feel we should "fix" this (e.g., by obtaining and forwarding the necessary parameters to the outlined function), we should certainly document this behavior.

Best,
Tobias

PS: Marking this as requesting changes to be informed about updates.

include/llvm/Transforms/Utils/Cloning.h
241

Maybe document ForwardVarArgsTo? How is it used? What is the use case, in which context is it valid to use, and how is code normally generated to require the use of this flag (outlining with AllowVarargs)?

include/llvm/Transforms/Utils/CodeExtractor.h
79

Document the new flag?

lib/Transforms/Utils/InlineFunction.cpp
1494

Document the new parameter?

1655

Unrelated!

1865

Start variable name with UpperCase?

This revision now requires changes to proceed.Nov 7 2017, 12:58 AM
fhahn updated this revision to Diff 121868.Nov 7 2017, 3:26 AM
fhahn edited edge metadata.
fhahn marked 4 inline comments as done.

Thanks Tobias! I've updated the patch trying to address your comments.

fhahn added a comment.Nov 7 2017, 3:29 AM

Also, AFAIKS the outliner will (temporarily) generate invalid IR as the calls to the outlined function won't forward the vararg parameters. This is only "fixed" after inlining, when the va_start() and va_end() intrinsics again refer to the right function. While I don't feel we should "fix" this (e.g., by obtaining and forwarding the necessary parameters to the outlined function), we should certainly document this behavior.

Correct. I've added a comment to doFunctionOutlining, not sure if that is the right place. I think we would also have to ensure that vaend is also in the outlined function?

lib/Transforms/Utils/InlineFunction.cpp
1494

Hm it looks like the same function is documented here and in include/llvm/Transforms/Utils/Cloning.h. The first 2 paragraphs match, but include/llvm/Transforms/Utils/Cloning.h contains additional info. According to [1], the doxygen comments for public functions should only be at the declaration.

[1] https://llvm.org/docs/CodingStandards.html#comment-formatting

Also, AFAIKS the outliner will (temporarily) generate invalid IR as the calls to the outlined function won't forward the vararg parameters. This is only "fixed" after inlining, when the va_start() and va_end() intrinsics again refer to the right function. While I don't feel we should "fix" this (e.g., by obtaining and forwarding the necessary parameters to the outlined function), we should certainly document this behavior.

Correct. I've added a comment to doFunctionOutlining, not sure if that is the right place.

That looks good to me.

I think we would also have to ensure that vaend is also in the outlined function?

Right. Both va_start and va_end need to be in the outlined function -- or more correct -- they cannot be in the part that is not outlined.

lib/Transforms/IPO/PartialInlining.cpp
835

As suggested by yourself, also check for va_end here.

grosser accepted this revision.Nov 7 2017, 5:26 AM

Officially accept this revision. From my perspective this looks good.

Do you have some other partial inliner person who could give some additional feedback?

This revision is now accepted and ready to land.Nov 7 2017, 5:26 AM
davidxl added inline comments.Nov 7 2017, 9:45 AM
lib/Transforms/IPO/PartialInlining.cpp
830

Is it possible to teach code extractor to handle this and return null instead of doing the cleanup here? CodeExtractor knows the set of blocks to be extracted or not.

fhahn updated this revision to Diff 122056.EditedNov 8 2017, 4:08 AM
fhahn marked an inline comment as done.

Added check for vaend and moved check to extractCodeRegion.

fhahn added a comment.Nov 8 2017, 4:09 AM

Officially accept this revision. From my perspective this looks good.

Do you have some other partial inliner person who could give some additional feedback?

I think it would be great if either @davidxl or @davide could sign-off on this.

lib/Transforms/IPO/PartialInlining.cpp
830

Yes I moved the code to the beginning of extractCodeRegion, before anything is modified. I could not find a place were CodeExtractor already iterates over the set of blocks not the be extracted, so it is iterating over the basic blocks of the original function and excludes blocks to be extracted.

Officially accept this revision. From my perspective this looks good.

Do you have some other partial inliner person who could give some additional feedback?

I think it would be great if either @davidxl or @davide could sign-off on this.

Yes, this would be good, indeed!

I also like the suggestion from @davidxl to move the check into the extractor.

davidxl edited edge metadata.Nov 8 2017, 8:51 AM

We do life-time marker and static alloca shrink wrapping optimization for outlining. Do you see opportunity for vaarg shrink-wrapping too? If yes, it may be considered as a follow up.

lib/Transforms/IPO/PartialInlining.cpp
153

This looks like stale comment.

lib/Transforms/Utils/CodeExtractor.cpp
82

Is there a need for this parameter?

119

Having the check here may prevent other independent legality check (in the future) from taking effect. If it better to move it down.

124

if (AllowVarArgs)

continue;

else

return false;
981

nit: use std::any_of

davide requested changes to this revision.Nov 8 2017, 2:01 PM

Really nice :) Some comments inline to begin with.

include/llvm/Transforms/Utils/CodeExtractor.h
92–93

This should presumably have a comment explaining what the additional bool controls.

lib/Transforms/Utils/CodeExtractor.cpp
177–178

Mind to add a comment next to the false with the member variable name :) ?

lib/Transforms/Utils/InlineFunction.cpp
1829–1830

Are you switching this because of iterator invalidation issues?
In this case, maybe it would be cleaner to collect the set of instructions to be removed in a set, and then remove them at the end of this loop?

This revision now requires changes to proceed.Nov 8 2017, 2:01 PM
fhahn updated this revision to Diff 122361.Nov 9 2017, 4:25 PM
fhahn edited edge metadata.
fhahn marked 4 inline comments as done.

Thank you very much for having a look. Updated to address the comments.

fhahn added inline comments.Nov 10 2017, 6:20 AM
lib/Transforms/IPO/PartialInlining.cpp
153

It still applies, the outliner will generate a call to outlined vararg function, but no varargs will be passed. When we do the inlining, the varargs from the original call site will be forwarded to the outlined function. Maybe that comment needs re-phrasing to make it more clear? Or maybe the comment should be at a different place?

lib/Transforms/Utils/CodeExtractor.cpp
82

Unfortunately isBlockValidForExtraction is a static member function, so I think we need this one.

davide accepted this revision.Nov 10 2017, 12:32 PM

LGTM too. Thanks Florian.

This revision is now accepted and ready to land.Nov 10 2017, 12:32 PM
fhahn closed this revision.Nov 13 2017, 2:35 AM