[PATCH] D100365: [AArch64] Fix windows vararg functions with floats in the fixed args

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp:174
+    auto &Subtarget = MF.getSubtarget<AArch64Subtarget>();
+    IsWin = Subtarget.isCallingConvWin64(MF.getFunction().getCallingConv());
+  }
----------------
I'm guessing this would become `UseVarArgsCCForFixed = IsWin && IsVarArg;`


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp:247
     bool Res;
-    if (Info.IsFixed)
+    if (Info.IsFixed && (!IsWin || !IsVarArg))
       Res = AssignFn(ValNo, ValVT, LocVT, LocInfo, Flags, State);
----------------
Really, I wanted to find a way to make this `if` more self-documenting.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp:259-260
   bool IsTailCall;
+  bool IsVarArg;
+  bool IsWin;
 
----------------
Let's pre-calculate a boolean, something like `UseVarArgsCCForFixed`, and put that in the `if` above.


================
Comment at: llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp:560-561
 static std::pair<CCAssignFn *, CCAssignFn *>
 getAssignFnsForCC(CallingConv::ID CC, const AArch64TargetLowering &TLI) {
   return {TLI.CCAssignFnForCall(CC, false), TLI.CCAssignFnForCall(CC, true)};
 }
----------------
mstorsjo wrote:
> mstorsjo wrote:
> > rnk wrote:
> > > Can we put the logic here?
> > Hmm, that might be possible, I'll have a go at it.
> Actually, it looks to me like it'd be more of a mess than what this patch is; `getAssignFnsForCC` is called in 5 places, and we'd need to have access to the MachineFunction (for querying the subtarget, whether the calling convention is a windows one, and for querying whether the function is a variadic one). E.g. in the case of doCallerAndCalleePassArgsTheSameWay below, we have the MachineFunction for the caller, but for the callee, we only have a `CallLoweringInfo` struct which only contains a MachineOperand.
I think AArch64TargetLowering has the subtarget, but I see that it is private. You can use TLI to get the TargetMachine, which will tell you if we are on Windows, but it doesn't have access to the `isCallingConvWin64` logic in AArch64Subtarget. That logic could be reimplemented inline here, but I think I agree, let's find another way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100365/new/

https://reviews.llvm.org/D100365