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

[Tim Northover via Phabricator via llvm-commits]

t.p.northover marked 4 inline comments as done.
t.p.northover added a comment.

Thanks for the comments. I've implemented a lot of them and explained the reasoning behind what I did for some. I'll upload a new patch.



================
Comment at: llvm/include/llvm/CodeGen/TargetFrameLowering.h:165
+                                           unsigned &MaxCSFrameIndex) const {
+    return assignCalleeSavedSpillSlots(MF, TRI, CSI);
+  }
----------------
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.


================
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)
----------------
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.


================
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)
----------------
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).


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1792
+        .addUse(AArch64::FP)
+        .addImm(0x10fe)
+        .setMIFlag(MachineInstr::FrameDestroy);
----------------
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.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:138
+  case AArch64::StoreSwiftAsyncContext:
+    NumBytes = 20;
+    break;
----------------
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).


================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp:641
 
+        if (CurOffset != 0 && Inst.getOffset() != CurOffset - 8)
+          return CU::UNWIND_ARM64_MODE_DWARF;
----------------
thegameg wrote:
> I'm having trouble seeing what this is checking for. I understand we want to fallback to DWARF like you explained in `llvm/test/CodeGen/AArch64/swift-async-unwind.ll`, but I'm not sure I get the way `CurOffset` is used to check for that.
Until now, this generator assumed that the .cfi directives it saw always started at an offset just after where FP was stored and counted up by 8 each time.

With the gap for the new context (and possibly padding) this is no longer true so we have to check that assumption and bail to DWARF if it's violated. This seemed like the least fragile  way to do that, even though the specific hole we're looking for could be checked more easily.


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

https://reviews.llvm.org/D95044