[PATCH] D37898: [TargetLowering] Correctly track NumFixedArgs field of CallLoweringInfo

[Alex Bradbury via Phabricator via llvm-commits]

asb created this revision.
Herald added subscribers: kristof.beyls, mehdi_amini, aemerson.

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.


https://reviews.llvm.org/D37898

Files:
  include/llvm/Target/TargetLowering.h
  lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp


Index: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
===================================================================
--- lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -8046,6 +8046,7 @@
     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
Index: include/llvm/Target/TargetLowering.h
===================================================================
--- include/llvm/Target/TargetLowering.h
+++ include/llvm/Target/TargetLowering.h
@@ -2906,7 +2906,7 @@
       RetTy = ResultType;
       Callee = Target;
       CallConv = CC;
-      NumFixedArgs = Args.size();
+      NumFixedArgs = ArgsList.size();
       Args = std::move(ArgsList);
 
       DAG.getTargetLoweringInfo().markLibCallAttributes(
@@ -2919,7 +2919,7 @@
       RetTy = ResultType;
       Callee = Target;
       CallConv = CC;
-      NumFixedArgs = Args.size();
+      NumFixedArgs = ArgsList.size();
       Args = std::move(ArgsList);
       return *this;
     }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D37898.115378.patch
Type: text/x-patch
Size: 1231 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170915/e9516f25/attachment.bin>