[PATCH] D39607: [PartialInliner] Inline vararg functions that forward varargs.

[Tobias Grosser via Phabricator via llvm-commits]

grosser requested changes to this revision.
grosser added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: include/llvm/Transforms/Utils/Cloning.h:236
+                    AAResults *CalleeAAR = nullptr, bool InsertLifetime = true,
+                    Function *ForwardVarArgsTo = nullptr);
 
----------------
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)?


================
Comment at: include/llvm/Transforms/Utils/CodeExtractor.h:74
+                  BranchProbabilityInfo *BPI = nullptr,
+                  bool AllowVarargs = false);
 
----------------
Document the new flag?


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1494
+                          AAResults *CalleeAAR, bool InsertLifetime,
+                          Function *ForwardVarArgsTo) {
   Instruction *TheCall = CS.getInstruction();
----------------
Document the new parameter?


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1655
     }
-
     // Add alignment assumptions if necessary. We do this before the inlined
----------------
Unrelated!


================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1864
+          Params.append(VarArgsToForward.begin(), VarArgsToForward.end());
+          CallInst *call = CallInst::Create(CI->getCalledFunction(), Params, "", CI);
+          CI->replaceAllUsesWith(call);
----------------
Start variable name with UpperCase?


https://reviews.llvm.org/D39607