[llvm-commits] [PATCH] Add va_argpack intrinsics
Benjamin Kramer
benny.kra at googlemail.com
Tue May 15 10:03:53 PDT 2012
On 15.05.2012, at 18:16, Chandler Carruth wrote:
> On Tue, May 15, 2012 at 9:08 AM, Benjamin Kramer <benny.kra at googlemail.com> wrote:
> This will be used to implement __builtin_va_arg_pack[len] in clang (PR7219).
>
> Awesome, thanks for working on this...
>
> Since this is used to model a pretty crazy gcc extension (used by glibc) we have
> to expand the intrinsics in the inliner. When it replaces a vararg call site with
> a copy of the function another pass over all instructions is performed which
> replaces any pack intrinsics with the values from the call site. There are some
> rather narrow requirements where the intrinsics can occur, documented in LangRef
> and enforced by the verifier.
>
> This patch has two other side effects:
> - DAE is not allowed to remove varargs if a va_pack intrinsic is used.
> - Inliner can now inline vararg functions iff they don't use va_start. This can
> happen if DAE doesn't remove the varargs because there is a va_pack intrinsic
> or if the AlwaysInliner tries to inline the function.
>
>
> I'm pretty OK wit this strategy. It actually has some other interesting artifacts I'd like to explore later. Specifically, I think we can generalize this to allow inlining of varargs functions when the va_start is in dead code...
>
> Comments on the patch:
> -------
>
> diff --git a/docs/LangRef.html b/docs/LangRef.html
> index 8328def..2c11366 100644
> --- a/docs/LangRef.html
> +++ b/docs/LangRef.html
> @@ -224,6 +224,8 @@
> <li><a href="#int_va_start">'<tt>llvm.va_start</tt>' Intrinsic</a></li>
> <li><a href="#int_va_end">'<tt>llvm.va_end</tt>' Intrinsic</a></li>
> <li><a href="#int_va_copy">'<tt>llvm.va_copy</tt>' Intrinsic</a></li>
> + <li><a href="#int_va_pack">'<tt>llvm.va_pack</tt>' Intrinsic</a></li>
> + <li><a href="#int_va_packlen">'<tt>llvm.va_packlen</tt>' Intrinsic</a></li>
>
> 'va_pack_len' would be more consistent with the GCC builtin... and as with the other varargs stuff, I think consistency is good here.
>
>
> @@ -6669,6 +6671,80 @@ declare void @llvm.va_end(i8*)
>
> </div>
>
> +<!-- _______________________________________________________________________ -->
> +<h4>
> + <a name="int_va_pack">'<tt>llvm.va_pack</tt>' Intrinsic</a>
> +</h4>
> +
> +<div>
> +
> +<h5>Syntax:</h5>
> +<pre>
> + declare i8* @llvm.va_pack()
> +</pre>
> +
> +<h5>Overview:</h5>
> +<p>The '<tt>llvm.va_pack</tt>' intrinsic expands varargs for a function call if
> +the caller is also a vararg function and is inlined. It can be used to implement
> +functionality like GCC's <tt>__builtin_va_arg_pack</tt>.</p>
> +
> +<h5>Semantics:</h5>
> +<p>The '<tt>llvm.va_pack</tt>' intrinsic may only occur as the last argument of
> +a vararg function call. The calling function must have local or
> +<tt>available_externally</tt> linkage so it is never emitted standalone.</p>
>
> It must also be marked 'always_inline', and its presence requires that the always inliner is run in order to generate correct code. I think this is important to require in the semantic model, as this intrinsic has *no* meaning unless inlining occurs.
>
>
> diff --git a/lib/Transforms/Utils/InlineFunction.cpp b/lib/Transforms/Utils/InlineFunction.cpp
> index d2b167a..32c5a17 100644
> --- a/lib/Transforms/Utils/InlineFunction.cpp
> +++ b/lib/Transforms/Utils/InlineFunction.cpp
> @@ -477,6 +477,84 @@ static void fixupLineNumbers(Function *Fn, Function::iterator FI,
> }
> }
>
> +// Expand va_pack and va_packlen intrinsics between FI and FE with the
> +// arguments from CS.
> +static void fixupArgPackInstrinsics(CallSite CS,
> + Function::iterator FI,
> + Function::iterator FE) {
> + if (!CS.getCalledFunction()->isVarArg())
> + return;
> +
> + // Get the number of varargs in the call.
> + unsigned NumNonVarArgs = CS.getCalledFunction()->arg_size();
> + unsigned NumVarArgs = CS.arg_size() - NumNonVarArgs;
> +
> + for (; FI != FE; ++FI)
> + for (BasicBlock::iterator BI = FI->begin(), BE = FI->end(); BI != BE;)
> + if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(BI++)) {
> + switch (II->getIntrinsicID()) {
> + default: break;
> + case Intrinsic::vapack:
>
> I would hoist each case's logic into a helper function -- 'for->for->if->switch->for' is too many levels of control for me in a single function. =]
>
> + for (Instruction::use_iterator UI = II->use_begin(),
> + UE = II->use_end(); UI != UE; ++UI) {
> + CallSite PackUser(*UI);
> +
> + // Create a new list of arguments. First the arguments from the call
> + // up to the va_pack intrinsic. Then the remaining arguments from
> + // the call site.
> + std::vector<Value *> NewArgs;
> + NewArgs.insert(NewArgs.end(), PackUser.arg_begin(),
> + llvm::prior(PackUser.arg_end()));
>
> Just construct the vector directly from these?
>
> + NewArgs.insert(NewArgs.end(), CS.arg_begin() + NumNonVarArgs,
> + CS.arg_end());
> +
> + // Now Create a call/invoke with the new arguments.
> + Instruction *New;
> + if (InvokeInst *II=dyn_cast<InvokeInst>(PackUser.getInstruction()))
> + New = InvokeInst::Create(II->getCalledFunction(),
> + II->getNormalDest(), II->getUnwindDest(),
> + NewArgs, II->getName(), II);
> + else
> + New = CallInst::Create(PackUser.getCalledFunction(), NewArgs,
> + PackUser->getName(),
> + PackUser.getInstruction());
> +
> + // And remove the old call.
> + if (&*BI == PackUser.getInstruction())
> + BI = New;
> + PackUser->replaceAllUsesWith(New);
> + PackUser->eraseFromParent();
> + }
> + // At this point all uses are replaced, remove the intrinsic.
> + II->eraseFromParent();
> + break;
> + case Intrinsic::vapacklen: {
> + ConstantInt *NumVarArgsCI =
> + ConstantInt::get(Type::getInt64Ty(CS.getType()->getContext()),
> + NumVarArgs);
> + II->replaceAllUsesWith(NumVarArgsCI);
> + II->eraseFromParent();
> + break;
> + }
> + }
> + }
> +}
> +
> +// We can still inline a var arg function if the variable arguments aren't
> +// actually used. Look explicitly for the vastart intrinsic.
>
> Can you add a fixme here? We shouldn't be doing this in an isolated for-loop inside the inliner.
>
> I'd like to have inline cost (both the always-inliner and the normal one) filter inlining when varargs are live in the function, and have the inliner actually just assert if it encounters one of these intrinsics.... But I don't think this needs to happen in this patch.
>
> +static bool usesVarArgs(const Function *CalledFunc) {
> + if (CalledFunc->isVarArg())
> + for (Function::const_iterator FI = CalledFunc->begin(),
> + FE = CalledFunc->end(); FI != FE; ++FI)
> + for (BasicBlock::const_iterator BI = FI->begin(), BE = FI->end();
> + BI != BE; ++BI)
> + if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(BI))
> + if (II->getIntrinsicID() == Intrinsic::vastart)
> + return true;
> +
> + return false;
> +}
> +
> /// InlineFunction - This function inlines the called function into the basic
> /// block of the caller. This returns false if it is not possible to inline
> /// this call. The program is still in a well defined state if this occurs
> @@ -498,7 +576,7 @@ bool llvm::InlineFunction(CallSite CS, InlineFunctionInfo &IFI,
> const Function *CalledFunc = CS.getCalledFunction();
> if (CalledFunc == 0 || // Can't inline external function or indirect
> CalledFunc->isDeclaration() || // call, or call to a vararg function!
> - CalledFunc->getFunctionType()->isVarArg()) return false;
> + usesVarArgs(CalledFunc)) return false;
>
> // If the call to the callee is not a tail call, we must clear the 'tail'
> // flags on any calls that we inline.
> @@ -568,9 +646,6 @@ bool llvm::InlineFunction(CallSite CS, InlineFunctionInfo &IFI,
> { // Scope to destroy VMap after cloning.
> ValueToValueMapTy VMap;
>
> - assert(CalledFunc->arg_size() == CS.arg_size() &&
> - "No varargs calls can be inlined!");
> -
> // Calculate the vector of arguments to pass into the function cloner, which
> // matches up the formal to the actual argument values.
> CallSite::arg_iterator AI = CS.arg_begin();
> @@ -721,6 +796,9 @@ bool llvm::InlineFunction(CallSite CS, InlineFunctionInfo &IFI,
> // return instruction, we splice the body of the inlined callee directly into
> // the calling basic block.
> if (Returns.size() == 1 && std::distance(FirstNewBlock, Caller->end()) == 1) {
> + // Expand va_pack intrinsics.
> + fixupArgPackInstrinsics(TheCall, FirstNewBlock, Caller->end());
> +
> // Move all of the instructions right before the call.
> OrigBB->getInstList().splice(TheCall, FirstNewBlock->getInstList(),
> FirstNewBlock->begin(), FirstNewBlock->end());
> @@ -785,6 +863,8 @@ bool llvm::InlineFunction(CallSite CS, InlineFunctionInfo &IFI,
> "splitBasicBlock broken!");
> Br->setOperand(0, FirstNewBlock);
>
> + // Expand va_pack intrinsics.
> + fixupArgPackInstrinsics(TheCall, FirstNewBlock, Caller->end());
>
> // Now that the function is correct, make it a little bit nicer. In
> // particular, move the basic blocks inserted from the end of the function
> diff --git a/lib/VMCore/Verifier.cpp b/lib/VMCore/Verifier.cpp
> index 47baef3..cc12140 100644
> --- a/lib/VMCore/Verifier.cpp
> +++ b/lib/VMCore/Verifier.cpp
> @@ -1769,6 +1769,24 @@ void Verifier::visitIntrinsicFunctionCall(Intrinsic::ID ID, CallInst &CI) {
> Assert1(isa<ConstantInt>(CI.getArgOperand(1)),
> "llvm.invariant.end parameter #2 must be a constant integer", &CI);
> break;
> + case Intrinsic::vapack:
> + for (Value::use_iterator UI = CI.use_begin(), UE = CI.use_end(); UI != UE;
> + ++UI) {
> + Assert1(isa<CallInst>(*UI) || isa<InvokeInst>(*UI),
> + "llvm.vapack is only allowed as an argument", &CI);
>
> "va_pack"... missing '_' here and below.
>
> + Assert1(CallSite(*UI).getCalledFunction()->isVarArg(),
> + "llvm.vapack must be used as a vararg argument", &CI);
> + Assert1(*llvm::prior(CallSite(*UI).arg_end()) == &CI,
> + "llvm.vapack must be the last argument in a call", &CI);
> + }
> + // FALLTHROUGH
> + case Intrinsic::vapacklen:
> + Function *F = CI.getParent()->getParent();
> + Assert1(F->isVarArg(),
> + "llvm.vapack must not be called outside of vararg functions", &CI);
> + Assert1(F->hasLocalLinkage() || F->hasAvailableExternallyLinkage(),
> + "Function containing llvm.vapack intrinsics must be inlined", &CI);
>
> We should also verify that the caller is always_inline.
PatchV2 with review comments addressed:
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-support-for-va_pack-and-va_packlen-intrinsic.patch
Type: application/octet-stream
Size: 17202 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120515/437e2a05/attachment.obj>
-------------- next part --------------
- Ben
More information about the llvm-commits
mailing list