Declare that musttail calls in variadic functions forward the ellipsis

[rnk]

Summary

There is no functionality change here except in the way we assemble and
dump musttail calls in variadic functions. There's really no need to
separate out the bits for musttail and "is forwarding varargs" on call
instructions. A musttail call by definition has to forward the ellipsis
or it would fail verification.

[rnk]

ping

[nlewycky]

I'm concerned that there's no Verifier change. It looks like you don't need one, which means that we failed at code review previously since there was code that was valid but we couldn't print or parse? Or is there a verifier change missing?

docs/LangRef.rst

6539–6541 ↗

(On Diff #12474)

"The callee must be varargs iff the caller is varargs"?

[rnk]

In D4892#5, @nlewycky wrote:

I'm concerned that there's no Verifier change. It looks like you don't need one, which means that we failed at code review previously since there was code that was valid but we couldn't print or parse? Or is there a verifier change missing?

We already have the check for this rule. I think you were the one who asked me to add it:

Assert1(CallerTy->isVarArg() == CalleeTy->isVarArg(),
        "cannot guarantee tail call due to mismatched varargs", &CI);

docs/LangRef.rst

6539–6541 ↗

(On Diff #12474)

sure

[rnk]

• Use iff

[nlewycky]

I haven't audited the codebase looking for places that assume that variadic callees must be passing in a discrete list of args, but every place I can think of (DAE, argpromotion, ipsccp) either skips varargs functions entirely or doesn't try to reason about the varargs part. Should be fine.

[rnk]

In D4892#9, @nlewycky wrote:

I haven't audited the codebase looking for places that assume that variadic callees must be passing in a discrete list of args, but every place I can think of (DAE, argpromotion, ipsccp) either skips varargs functions entirely or doesn't try to reason about the varargs part. Should be fine.

I see a problem in DAE, actually. I'll handle that in a separate commit. The test case for that feature is based on grep and needs work.

[rnk]

Closed by commit rL216423 (authored by @rnk).

llvm/trunk/docs/LangRef.rst

#. The optional ``tail`` and ``musttail`` markers indicate that the optimizers
    - The ret instruction must return the (possibly bitcasted) value
      produced by the call or void.
    - The caller and callee prototypes must match.  Pointer types of
      parameters or return types may differ in pointee type, but not
      in address space.
    - The calling conventions of the caller and callee must match.
    - All ABI-impacting function attributes, such as sret, byval, inreg,
      returned, and inalloca, must match.
+   - The callee must be varargs iff the caller is varargs. Bitcasting a
+     non-varargs function to the appropriate varargs type is legal so
+     long as the non-varargs prefixes obey the other rules.
 
    Tail call optimization for calls marked ``tail`` is guaranteed to occur if
    the following conditions are met:
 
    -  Caller and callee both have the calling convention ``fastcc``.
    -  The call is in tail position (ret immediately follows call and ret
       uses value of call or is void).
    -  Option ``-tailcallopt`` is enabled, or

llvm/trunk/lib/AsmParser/LLParser.h

private:
     struct ParamInfo {
       LocTy Loc;
       Value *V;
       AttributeSet Attrs;
       ParamInfo(LocTy loc, Value *v, AttributeSet attrs)
         : Loc(loc), V(v), Attrs(attrs) {}
     };
     bool ParseParameterList(SmallVectorImpl<ParamInfo> &ArgList,
-                            PerFunctionState &PFS);
+                            PerFunctionState &PFS,
+                            bool IsMustTailCall = false,
+                            bool InVarArgsFunc = false);
 
     // Constant Parsing.
     bool ParseValID(ValID &ID, PerFunctionState *PFS = nullptr);
     bool ParseGlobalValue(Type *Ty, Constant *&V);
     bool ParseGlobalTypeAndValue(Constant *&V);
     bool ParseGlobalValueVector(SmallVectorImpl<Constant *> &Elts);
     bool parseOptionalComdat(Comdat *&C);
     bool ParseMetadataListValue(ValID &ID, PerFunctionState *PFS);

llvm/trunk/lib/AsmParser/LLParser.cpp

 }
 
 /// ParseParameterList
 ///    ::= '(' ')'
 ///    ::= '(' Arg (',' Arg)* ')'
 ///  Arg
 ///    ::= Type OptionalAttributes Value OptionalAttributes
 bool LLParser::ParseParameterList(SmallVectorImpl<ParamInfo> &ArgList,
-                                  PerFunctionState &PFS) {
+                                  PerFunctionState &PFS, bool IsMustTailCall,
+                                  bool InVarArgsFunc) {
   if (ParseToken(lltok::lparen, "expected '(' in call"))
     return true;
 
   unsigned AttrIndex = 1;
   while (Lex.getKind() != lltok::rparen) {
     // If this isn't the first argument, we need a comma.
     if (!ArgList.empty() &&
         ParseToken(lltok::comma, "expected ',' in argument list"))
       return true;
 
+    // Parse an ellipsis if this is a musttail call in a variadic function.
+    if (Lex.getKind() == lltok::dotdotdot) {
+      const char *Msg = "unexpected ellipsis in argument list for ";
+      if (!IsMustTailCall)
+        return TokError(Twine(Msg) + "non-musttail call");
+      if (!InVarArgsFunc)
+        return TokError(Twine(Msg) + "musttail call in non-varargs function");
+      Lex.Lex();  // Lex the '...', it is purely for readability.
+      return ParseToken(lltok::rparen, "expected ')' at end of argument list");
+    }
+
     // Parse the argument.
     LocTy ArgLoc;
     Type *ArgTy = nullptr;
     AttrBuilder ArgAttrs;
     Value *V;
     if (ParseType(ArgTy, ArgLoc))
       return true;
 
     // Otherwise, handle normal operands.
     if (ParseOptionalParamAttrs(ArgAttrs) || ParseValue(ArgTy, V, PFS))
       return true;
     ArgList.push_back(ParamInfo(ArgLoc, V, AttributeSet::get(V->getContext(),
                                                              AttrIndex++,
                                                              ArgAttrs)));
   }
 
+  if (IsMustTailCall && InVarArgsFunc)
+    return TokError("expected '...' at end of argument list for musttail call "
+                    "in varargs function");
+
   Lex.Lex();  // Lex the ')'.
   return false;
 }
 
 
 
 /// ParseArgumentList - Parse the argument list for a function type or function
 /// prototype.
bool LLParser::ParseCall(Instruction *&Inst, PerFunctionState &PFS,
   LocTy CallLoc = Lex.getLoc();
 
   if ((TCK != CallInst::TCK_None &&
        ParseToken(lltok::kw_call, "expected 'tail call'")) ||
       ParseOptionalCallingConv(CC) ||
       ParseOptionalReturnAttrs(RetAttrs) ||
       ParseType(RetType, RetTypeLoc, true /*void allowed*/) ||
       ParseValID(CalleeID) ||
-      ParseParameterList(ArgList, PFS) ||
+      ParseParameterList(ArgList, PFS, TCK == CallInst::TCK_MustTail,
+                         PFS.getFunction().isVarArg()) ||
       ParseFnAttributeValuePairs(FnAttrs, FwdRefAttrGrps, false,
                                  BuiltinLoc))
     return true;
 
   // If RetType is a non-function pointer type, then this is the short syntax
   // for the call, which means that RetType is just the return type.  Infer the
   // rest of the function argument types from the arguments that are present.
   PointerType *PFTy = nullptr;

llvm/trunk/lib/IR/AsmWriter.cpp

if (!FTy->isVarArg() &&
       writeOperand(Operand, true);
     }
     Out << '(';
     for (unsigned op = 0, Eop = CI->getNumArgOperands(); op < Eop; ++op) {
       if (op > 0)
         Out << ", ";
       writeParamOperand(CI->getArgOperand(op), PAL, op + 1);
     }
+
+    // Emit an ellipsis if this is a musttail call in a vararg function.  This
+    // is only to aid readability, musttail calls forward varargs by default.
+    if (CI->isMustTailCall() && CI->getParent() &&
+        CI->getParent()->getParent() &&
+        CI->getParent()->getParent()->isVarArg())
+      Out << ", ...";
+
     Out << ')';
     if (PAL.hasAttributes(AttributeSet::FunctionIndex))
       Out << " #" << Machine.getAttributeGroupSlot(PAL.getFnAttributes());
   } else if (const InvokeInst *II = dyn_cast<InvokeInst>(&I)) {
     Operand = II->getCalledValue();
     PointerType *PTy = cast<PointerType>(Operand->getType());
     FunctionType *FTy = cast<FunctionType>(PTy->getElementType());
     Type *RetTy = FTy->getReturnType();

llvm/trunk/test/Assembler/musttail-invalid-1.ll

+; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
+
+; Check the error message on using ", ..." when we can't actually forward
+; varargs.
+
+%struct.A = type { i32 }
+
+declare i8* @f(i8*, ...)
+
+define i8* @f_thunk(i8* %this) {
+  %rv = musttail call i8* (i8*, ...)* @f(i8* %this, ...)
+; CHECK: error: unexpected ellipsis in argument list for musttail call in non-varargs function
+  ret i8* %rv
+}

llvm/trunk/test/Assembler/musttail-invalid-2.ll

+; RUN: not llvm-as < %s -o /dev/null 2>&1 | FileCheck %s
+
+; Check the error message on skipping ", ..." at the end of a musttail call argument list.
+
+%struct.A = type { i32 }
+
+declare i8* @f(i8*, ...)
+
+define i8* @f_thunk(i8* %this, ...) {
+  %rv = musttail call i8* (i8*, ...)* @f(i8* %this)
+; CHECK: error: expected '...' at end of argument list for musttail call in varargs function
+  ret i8* %rv
+}

llvm/trunk/test/Assembler/musttail.ll

+; RUN: llvm-as < %s | llvm-dis | FileCheck %s
+
+; Check that the ellipsis round trips.
+
+%struct.A = type { i32 }
+
+declare i8* @f(i8*, ...)
+
+define i8* @f_thunk(i8* %this, ...) {
+  %rv = musttail call i8* (i8*, ...)* @f(i8* %this, ...)
+  ret i8* %rv
+}
+; CHECK-LABEL: define i8* @f_thunk(i8* %this, ...)
+; CHECK: %rv = musttail call i8* (i8*, ...)* @f(i8* %this, ...)

llvm/trunk/test/Transforms/Inline/inline-musttail-varargs.ll

+; RUN: opt < %s -inline -instcombine -S | FileCheck %s
+
+; We can't inline this thunk yet, but one day we will be able to.  And when we
+; do, this test case will be ready.
+
+declare void @ext_method(i8*, i32)
+
+define linkonce_odr void @thunk(i8* %this, ...) {
+  %this_adj = getelementptr i8* %this, i32 4
+  musttail call void (i8*, ...)* bitcast (void (i8*, i32)* @ext_method to void (i8*, ...)*)(i8* %this_adj, ...)
+  ret void
+}
+
+define void @thunk_caller(i8* %p) {
+  call void (i8*, ...)* @thunk(i8* %p, i32 42)
+  ret void
+}
+; CHECK-LABEL: define void @thunk_caller(i8* %p)
+; CHECK: call void (i8*, ...)* @thunk(i8* %p, i32 42)
+
+; FIXME: Inline the thunk. This should be significantly easier than inlining
+; general varargs functions.