<div class="gmail_quote">On Tue, May 15, 2012 at 9:08 AM, Benjamin Kramer <span dir="ltr"><<a href="mailto:benny.kra@googlemail.com" target="_blank">benny.kra@googlemail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This will be used to implement __builtin_va_arg_pack[len] in clang (PR7219).<br></blockquote><div><br></div><div>Awesome, thanks for working on this...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Since this is used to model a pretty crazy gcc extension (used by glibc) we have<br>
to expand the intrinsics in the inliner. When it replaces a vararg call site with<br>
a copy of the function another pass over all instructions is performed which<br>
replaces any pack intrinsics with the values from the call site. There are some<br>
rather narrow requirements where the intrinsics can occur, documented in LangRef<br>
and enforced by the verifier.<br>
<br>
This patch has two other side effects:<br>
- DAE is not allowed to remove varargs if a va_pack intrinsic is used.<br>
- Inliner can now inline vararg functions iff they don't use va_start. This can<br>
happen if DAE doesn't remove the varargs because there is a va_pack intrinsic<br>
or if the AlwaysInliner tries to inline the function.<br></blockquote><div><br></div><div><br></div><div>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...</div>
<div><br></div><div>Comments on the patch:</div><div>-------</div><div><br></div><div><div>diff --git a/docs/LangRef.html b/docs/LangRef.html</div><div>index 8328def..2c11366 100644</div><div>--- a/docs/LangRef.html</div>
<div>+++ b/docs/LangRef.html</div><div>@@ -224,6 +224,8 @@</div><div> <li><a href="#int_va_start">'<tt>llvm.va_start</tt>' Intrinsic</a></li></div><div> <li><a href="#int_va_end">'<tt>llvm.va_end</tt>' Intrinsic</a></li></div>
<div> <li><a href="#int_va_copy">'<tt>llvm.va_copy</tt>' Intrinsic</a></li></div><div>+ <li><a href="#int_va_pack">'<tt>llvm.va_pack</tt>' Intrinsic</a></li></div>
<div>+ <li><a href="#int_va_packlen">'<tt>llvm.va_packlen</tt>' Intrinsic</a></li></div><div><br></div><div>'va_pack_len' would be more consistent with the GCC builtin... and as with the other varargs stuff, I think consistency is good here.</div>
<div><br></div><div><br></div><div>@@ -6669,6 +6671,80 @@ declare void @llvm.va_end(i8*)</div><div> </div><div> </div></div><div> </div><div>+<!-- _______________________________________________________________________ --></div>
<div>+<h4></div><div>+ <a name="int_va_pack">'<tt>llvm.va_pack</tt>' Intrinsic</a></div><div>+</h4></div><div>+</div><div>+<div></div><div>+</div><div>+<h5>Syntax:</h5></div>
<div>+<pre></div><div>+ declare i8* @llvm.va_pack()</div><div>+</pre></div><div>+</div><div>+<h5>Overview:</h5></div><div>+<p>The '<tt>llvm.va_pack</tt>' intrinsic expands varargs for a function call if</div>
<div>+the caller is also a vararg function and is inlined. It can be used to implement</div><div>+functionality like GCC's <tt>__builtin_va_arg_pack</tt>.</p></div><div>+</div><div>+<h5>Semantics:</h5></div>
<div>+<p>The '<tt>llvm.va_pack</tt>' intrinsic may only occur as the last argument of</div><div>+a vararg function call. The calling function must have local or</div><div>+<tt>available_externally</tt> linkage so it is never emitted standalone.</p></div>
<div><br></div><div>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.</div>
<div><br></div><div><br></div><div>diff --git a/lib/Transforms/Utils/InlineFunction.cpp b/lib/Transforms/Utils/InlineFunction.cpp</div><div>index d2b167a..32c5a17 100644</div><div>--- a/lib/Transforms/Utils/InlineFunction.cpp</div>
<div>+++ b/lib/Transforms/Utils/InlineFunction.cpp</div><div>@@ -477,6 +477,84 @@ static void fixupLineNumbers(Function *Fn, Function::iterator FI,</div><div> }</div><div> }</div><div> </div><div>+// Expand va_pack and va_packlen intrinsics between FI and FE with the</div>
<div>+// arguments from CS.</div><div>+static void fixupArgPackInstrinsics(CallSite CS,</div><div>+ Function::iterator FI,</div><div>+ Function::iterator FE) {</div>
<div>+ if (!CS.getCalledFunction()->isVarArg())</div><div>+ return;</div><div>+</div><div>+ // Get the number of varargs in the call.</div><div>+ unsigned NumNonVarArgs = CS.getCalledFunction()->arg_size();</div>
<div>+ unsigned NumVarArgs = CS.arg_size() - NumNonVarArgs;</div><div>+</div><div>+ for (; FI != FE; ++FI)</div><div>+ for (BasicBlock::iterator BI = FI->begin(), BE = FI->end(); BI != BE;)</div><div>+ if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(BI++)) {</div>
<div>+ switch (II->getIntrinsicID()) {</div><div>+ default: break;</div><div>+ case Intrinsic::vapack:</div><div><br></div><div>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. =]</div>
<div><br></div><div>+ for (Instruction::use_iterator UI = II->use_begin(),</div><div>+ UE = II->use_end(); UI != UE; ++UI) {</div><div>+ CallSite PackUser(*UI);</div><div>+</div><div>
+ // Create a new list of arguments. First the arguments from the call</div><div>+ // up to the va_pack intrinsic. Then the remaining arguments from</div><div>+ // the call site.</div><div>
+ std::vector<Value *> NewArgs;</div><div>+ NewArgs.insert(NewArgs.end(), PackUser.arg_begin(),</div><div>+ llvm::prior(PackUser.arg_end()));</div><div><br></div><div>
Just construct the vector directly from these?</div><div><br></div><div>+ NewArgs.insert(NewArgs.end(), CS.arg_begin() + NumNonVarArgs,</div><div>+ CS.arg_end());</div><div>+</div><div>
+ // Now Create a call/invoke with the new arguments.</div><div>+ Instruction *New;</div><div>+ if (InvokeInst *II=dyn_cast<InvokeInst>(PackUser.getInstruction()))</div><div>+ New = InvokeInst::Create(II->getCalledFunction(),</div>
<div>+ II->getNormalDest(), II->getUnwindDest(),</div><div>+ NewArgs, II->getName(), II);</div><div>+ else</div><div>+ New = CallInst::Create(PackUser.getCalledFunction(), NewArgs,</div>
<div>+ PackUser->getName(),</div><div>+ PackUser.getInstruction());</div><div>+</div><div>+ // And remove the old call.</div><div>+ if (&*BI == PackUser.getInstruction())</div>
<div>+ BI = New;</div><div>+ PackUser->replaceAllUsesWith(New);</div><div>+ PackUser->eraseFromParent();</div><div>+ }</div><div>+ // At this point all uses are replaced, remove the intrinsic.</div>
<div>+ II->eraseFromParent();</div><div>+ break;</div><div>+ case Intrinsic::vapacklen: {</div><div>+ ConstantInt *NumVarArgsCI =</div><div>+ ConstantInt::get(Type::getInt64Ty(CS.getType()->getContext()),</div>
<div>+ NumVarArgs);</div><div>+ II->replaceAllUsesWith(NumVarArgsCI);</div><div>+ II->eraseFromParent();</div><div>+ break;</div><div>+ }</div><div>+ }</div>
<div>+ }</div><div>+}</div><div>+</div><div>+// We can still inline a var arg function if the variable arguments aren't</div><div>+// actually used. Look explicitly for the vastart intrinsic.</div><div><br></div>
<div>Can you add a fixme here? We shouldn't be doing this in an isolated for-loop inside the inliner.</div><div><br></div><div>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.</div>
<div><br></div><div>+static bool usesVarArgs(const Function *CalledFunc) {</div><div>+ if (CalledFunc->isVarArg())</div><div>+ for (Function::const_iterator FI = CalledFunc->begin(),</div><div>+ FE = CalledFunc->end(); FI != FE; ++FI)</div>
<div>+ for (BasicBlock::const_iterator BI = FI->begin(), BE = FI->end();</div><div>+ BI != BE; ++BI)</div><div>+ if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(BI))</div><div>+ if (II->getIntrinsicID() == Intrinsic::vastart)</div>
<div>+ return true;</div><div>+</div><div>+ return false;</div><div>+}</div><div>+</div><div> /// InlineFunction - This function inlines the called function into the basic</div><div> /// block of the caller. This returns false if it is not possible to inline</div>
<div> /// this call. The program is still in a well defined state if this occurs</div><div>@@ -498,7 +576,7 @@ bool llvm::InlineFunction(CallSite CS, InlineFunctionInfo &IFI,</div><div> const Function *CalledFunc = CS.getCalledFunction();</div>
<div> if (CalledFunc == 0 || // Can't inline external function or indirect</div><div> CalledFunc->isDeclaration() || // call, or call to a vararg function!</div><div>- CalledFunc->getFunctionType()->isVarArg()) return false;</div>
<div>+ usesVarArgs(CalledFunc)) return false;</div><div> </div><div> // If the call to the callee is not a tail call, we must clear the 'tail'</div><div> // flags on any calls that we inline.</div><div>@@ -568,9 +646,6 @@ bool llvm::InlineFunction(CallSite CS, InlineFunctionInfo &IFI,</div>
<div> { // Scope to destroy VMap after cloning.</div><div> ValueToValueMapTy VMap;</div><div> </div><div>- assert(CalledFunc->arg_size() == CS.arg_size() &&</div><div>- "No varargs calls can be inlined!");</div>
<div>-</div><div> // Calculate the vector of arguments to pass into the function cloner, which</div><div> // matches up the formal to the actual argument values.</div><div> CallSite::arg_iterator AI = CS.arg_begin();</div>
<div>@@ -721,6 +796,9 @@ bool llvm::InlineFunction(CallSite CS, InlineFunctionInfo &IFI,</div><div> // return instruction, we splice the body of the inlined callee directly into</div><div> // the calling basic block.</div>
<div> if (Returns.size() == 1 && std::distance(FirstNewBlock, Caller->end()) == 1) {</div><div>+ // Expand va_pack intrinsics.</div><div>+ fixupArgPackInstrinsics(TheCall, FirstNewBlock, Caller->end());</div>
<div>+</div><div> // Move all of the instructions right before the call.</div><div> OrigBB->getInstList().splice(TheCall, FirstNewBlock->getInstList(),</div><div> FirstNewBlock->begin(), FirstNewBlock->end());</div>
<div>@@ -785,6 +863,8 @@ bool llvm::InlineFunction(CallSite CS, InlineFunctionInfo &IFI,</div><div> "splitBasicBlock broken!");</div><div> Br->setOperand(0, FirstNewBlock);</div><div> </div><div>
+ // Expand va_pack intrinsics.</div><div>+ fixupArgPackInstrinsics(TheCall, FirstNewBlock, Caller->end());</div><div> </div><div> // Now that the function is correct, make it a little bit nicer. In</div><div> // particular, move the basic blocks inserted from the end of the function</div>
<div>diff --git a/lib/VMCore/Verifier.cpp b/lib/VMCore/Verifier.cpp</div><div>index 47baef3..cc12140 100644</div><div>--- a/lib/VMCore/Verifier.cpp</div><div>+++ b/lib/VMCore/Verifier.cpp</div><div>@@ -1769,6 +1769,24 @@ void Verifier::visitIntrinsicFunctionCall(Intrinsic::ID ID, CallInst &CI) {</div>
<div> Assert1(isa<ConstantInt>(CI.getArgOperand(1)),</div><div> "llvm.invariant.end parameter #2 must be a constant integer", &CI);</div><div> break;</div><div>+ case Intrinsic::vapack:</div>
<div>+ for (Value::use_iterator UI = CI.use_begin(), UE = CI.use_end(); UI != UE;</div><div>+ ++UI) {</div><div>+ Assert1(isa<CallInst>(*UI) || isa<InvokeInst>(*UI),</div><div>+ "llvm.vapack is only allowed as an argument", &CI);</div>
<div><br></div><div>"va_pack"... missing '_' here and below.</div><div><br></div><div>+ Assert1(CallSite(*UI).getCalledFunction()->isVarArg(),</div><div>+ "llvm.vapack must be used as a vararg argument", &CI);</div>
<div>+ Assert1(*llvm::prior(CallSite(*UI).arg_end()) == &CI,</div><div>+ "llvm.vapack must be the last argument in a call", &CI);</div><div>+ }</div><div>+ // FALLTHROUGH</div><div>
+ case Intrinsic::vapacklen:</div><div>+ Function *F = CI.getParent()->getParent();</div><div>+ Assert1(F->isVarArg(),</div><div>+ "llvm.vapack must not be called outside of vararg functions", &CI);</div>
<div>+ Assert1(F->hasLocalLinkage() || F->hasAvailableExternallyLinkage(),</div><div>+ "Function containing llvm.vapack intrinsics must be inlined", &CI);</div><div><br></div><div>We should also verify that the caller is always_inline.</div>
</div></div>