[TargetLowering] Correctly track NumFixedArgs field of CallLoweringInfo

[asb]

Summary

This is a really trivial change, however I am flagging it up via code review before committing because there is a non-zero chance that it could cause subtle ABI-related issues for libcall lowering on other backends (though all LLVM and Clang tests pass for all in-tree default backends). I've added you as a reviewer if your backend accesses the IsFixed field of ISD::OutputArg regardless of the value of CLI.IsVarArg (AArch64, ARM, SystemZ, X86). Depending on your target's calling convention implementation, this fix could change the behaviour for lowering some libcalls. For instance, on RISC-V before this fix any libcall was lowered according to the vararg calling convention. This is almost never a problem, but does cause issues for %1 = sitofp i64 %a to fp128 which gets lowered to a __floatditf libcall. On RV32, a0 would hold a pointer to the return address and a1+a2 should hold the i64 argument. But by the vararg calling convention, a2+a3 would hold the i64 argument instead ('aligned' register pairs must be used).

I think it's unlikely this will have any observable change on other backends, but think it's best to flag that possibility now rather than have people waste time tracking subtle breakages in a few months time.

More detailed explanation of the changes below:

The NumFixedArgs field of CallLoweringInfo is used by TargetLowering::LowerCallTo to determine whether a given argument is passed using the vararg calling convention or not (specifically, to set IsFixed for each ISD::OutputArg).

Firstly, CallLoweringInfo::setLibCallee and CallLoweringInfo::setCallee both incorrectly set NumFixedArgs based on the _previous_ args list. Secondly, TargetLowering::LowerCallTo failed to increment NumFixedArgs when modifying the argument list so a pointer is passed for the return value.

If your backend uses the IsFixed property, it is possible this change could result in codegen changes (although the previous behaviour would have been incorrect). This codepath is primarily exercised when lowering libcalls.

[uweigand]

As far as SystemZ is concerned, this change should be safe. In our ABI only vector arguments are affected by the IsFixed flags, and there shouldn't be any libcalls with vector arguments.

In theory, the patch could affect non-libcalls in those cases where the return type must be passed by implicit reference. But at least in Clang, that transformation is already done by Clang itself, so this LLVM code path is never hit from within Clang. I believe none of the non-Clang users of LLVM on SystemZ currently hit this code path either (since it was erroneously missing for a long time in the SystemZ back-end and nobody complained ...).

The only other code paths seem to affect statepoints and patchpoints, so if one of those has vector arguments, we may now get different code. But those features are marked experimental, so we should be fine here.

In any case, the new code implements the ABI correctly where the old code did not, so in my opinion the change needs to be done anyway for compatibility with other compilers.

[asb]

Ping? Any concerns from an ARM/AArch64/X86 perspective?

[efriedma]

x86, arm, and aarch64 have calling conventions which care whether a call is varargs, but not whether a particular argument is specified in the prototype.

Some grepping shows that MIPS, WebAssembly and Lanai care about NumFixedArgs.

[dschuff]

Thanks for the heads-up. WebAssembly's current varargs calling convention does pass the fixed args in one way (via declared function arguments) and the non-fixed args in another way (in memory), so in the cases where NumFixedArgs was wrong we would have been doing the wrong thing. However wasm also typechecks all calls against the declared function signature, so any wrong cases would fail in obvious ways (a signature mismatch error at validation time for direct calls, and at runtime for indirect calls). So just making the behavior correct should be the right thing to do. Also we have explicitly promised that we don't have a stable ABI yet, so that helps too :D

So, LGTM.

[asb]

So SystemZ and WebAssembly are happy. Do any other backend maintainers have any concerns? Thanks Ulrich for pointing out that I should have been grepping for NumFixedArgs accesses in addition to IsFixed.

[efriedma]

LGTM

llvm/trunk/include/llvm/Target/TargetLowering.h

struct CallLoweringInfo {
     }
 
     // setCallee with target/module-specific attributes
     CallLoweringInfo &setLibCallee(CallingConv::ID CC, Type *ResultType,
                                    SDValue Target, ArgListTy &&ArgsList) {
       RetTy = ResultType;
       Callee = Target;
       CallConv = CC;
-      NumFixedArgs = Args.size();
+      NumFixedArgs = ArgsList.size();
       Args = std::move(ArgsList);
 
       DAG.getTargetLoweringInfo().markLibCallAttributes(
           &(DAG.getMachineFunction()), CC, Args);
       return *this;
     }
 
     CallLoweringInfo &setCallee(CallingConv::ID CC, Type *ResultType,
                                 SDValue Target, ArgListTy &&ArgsList) {
       RetTy = ResultType;
       Callee = Target;
       CallConv = CC;
-      NumFixedArgs = Args.size();
+      NumFixedArgs = ArgsList.size();
       Args = std::move(ArgsList);
       return *this;
     }
 
     CallLoweringInfo &setCallee(Type *ResultType, FunctionType *FTy,
                                 SDValue Target, ArgListTy &&ArgsList,
                                 ImmutableCallSite Call) {
       RetTy = ResultType;

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

if (!CanLowerReturn) {
     Entry.IsSRet = true;
     Entry.IsNest = false;
     Entry.IsByVal = false;
     Entry.IsReturned = false;
     Entry.IsSwiftSelf = false;
     Entry.IsSwiftError = false;
     Entry.Alignment = Align;
     CLI.getArgs().insert(CLI.getArgs().begin(), Entry);
+    CLI.NumFixedArgs += 1;
     CLI.RetTy = Type::getVoidTy(CLI.RetTy->getContext());
 
     // sret demotion isn't compatible with tail-calls, since the sret argument
     // points into the callers stack frame.
     CLI.IsTailCall = false;
   } else {
     for (unsigned I = 0, E = RetTys.size(); I != E; ++I) {
       EVT VT = RetTys[I];