[PATCH] D35006: [AArch64] Implement support for windows style vararg functions

[Tim Northover via Phabricator via llvm-commits]

t.p.northover added inline comments.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:2889
+    if (Subtarget->isTargetWindows())
+      GPRIdx = MFI.CreateFixedObject(GPRSaveSize, GPRSaveSize & 15, false);
+    else
----------------
I don't understand this line, particularly the `GPRSaveSize & 15`. I'd expected to see `-GPRSaveSize` since the args are on the stack just before the canonical frame address (i.e. SP on function entry).

I assume the masking is a crude version of `alignTo`? I don't think that's going to be right. You need to make sure the stack is aligned in FrameLowering, but you're still going to want the FrameIndex that gets put into a va_list is able to point an odd number of registers below SP on entry if necessary.

Things look equally bad when I dump the MIR (e.g. from your first test-case):

    [...]
    fi#-1: size=56, align=8, fixed, at location [SP+8]
    [...]
    STRXui %X1<kill>, <fi#-1>, 0; mem:ST8[FixedStack-1+8]

So LLVM thinks X1 is going at the incoming SP+8 rather than (as actually happens) incoming SP-56 (I think).

So I think something is going horribly wrong during FrameLowering and LLVM is losing track of where SP actually is compared to function entry. I'll try to take a look again later, but I've not spotted the issue yet.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:2901-2902
+          Subtarget->isTargetWindows()
+              ? MachinePointerInfo::getFixedStack(DAG.getMachineFunction(),
+                                                  GPRIdx, 8)
+              : MachinePointerInfo::getStack(DAG.getMachineFunction(), i * 8));
----------------
I think this is lying about the location isn't it? They're not all being stored at GPRIdx.


https://reviews.llvm.org/D35006