[PATCH] D95044: IR+AArch64: add `swiftasync` function parameter attribute

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs accepted this revision.
jroelofs added a comment.

LGTM



================
Comment at: llvm/include/llvm/CodeGen/TargetFrameLowering.h:165
+                                           unsigned &MaxCSFrameIndex) const {
+    return assignCalleeSavedSpillSlots(MF, TRI, CSI);
+  }
----------------
t.p.northover wrote:
> jroelofs wrote:
> > Are callers expected to know whether the implementation of this API sets `{Min,Max}CSFrameIndex`? If there's a way to derive them, or set them to some default value, I think that would make this easier to hold.
> > 
> > Maybe it's better to implement the other one in terms of this one, to force them to be correctly populated even if they're not used?
> There's just one caller of this function, in PrologEpilogInserter.cpp, and it calls this new version. Targets that want to set the new variables will override this one, targets that don't will override the other one.
> 
> If the target doesn't set them,  PrologEpilogInserter supplies defaults (essentially saying no registers count as callee-saved) and I believe that changes some details like spill order.
> 
> I'm not sure if there's a point to that, just explaining some of the background really.
ok, sounds good.


================
Comment at: llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:711
+  // move it somewhere before signing.
+  BuildMI(MBB, MBBI, DL, TII->get(AArch64::ORRXrs), AArch64::X17)
+      .addUse(AArch64::XZR)
----------------
t.p.northover wrote:
> jroelofs wrote:
> > If it's supposed to be zeroing `X17`, wouldn't `MOVZ` be better?
> > 
> > The comment above suggests it should be doing a `mov x17, x22` though, and neither behavior matches what the code is doing here.
> On AArch64, `mov xD, xS` is actually just an alias for the instruction fully described as `orr xD, xzr, xS, lsl #0` which is this what we're doing here (`mov x17, CtxReg`). I think that matches the comment.
ack


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1146
+  if (HasFP && AFI->hasSwiftAsyncContext()) {
+    // ORR x29, x29, #0x1000_0000_0000_0000
+    BuildMI(MBB, MBBI, DL, TII->get(AArch64::ORRXri), AArch64::FP)
----------------
t.p.northover wrote:
> jroelofs wrote:
> > do you mean `#0x1000_1000_0000_0000`?
> No, it's just that one bit. The encoding for AArch64 bitwise immediates is weird (it allows weird repeating patterns etc).
ack


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1792
+        .addUse(AArch64::FP)
+        .addImm(0x10fe)
+        .setMIFlag(MachineInstr::FrameDestroy);
----------------
t.p.northover wrote:
> jroelofs wrote:
> > I'm having trouble making sense of this immediate. Comment above says it clears the MSB, but this looks like it does more than that.
> Same as above, weird encoding for immediates. Hence the comment, really.
ack


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:2907
+
+    int FrameIdx;
+    unsigned Size = RegInfo->getSpillSize(*RC);
----------------
t.p.northover wrote:
> jroelofs wrote:
> > sink the declaration of `FrameIdx` to its definition.
> Promise I have done this, I just forgot to save the file before committing when I uploaded the new diff.
ack


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:138
+  case AArch64::StoreSwiftAsyncContext:
+    NumBytes = 20;
+    break;
----------------
t.p.northover wrote:
> jroelofs wrote:
> > This is for that `pacdb` sequence right? Might be worth a comment here showing what the 5 instructions are.
> This switch is notorious for getting out of date when expansion changes. But the biggest problem isn't people not knowing what a pseudo might expand to; it's forgetting the switch exists at all.
> 
> A comment here is just one more thing to go stale rather than a useful thing you might act on (IMO).
ack


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

https://reviews.llvm.org/D95044