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

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs added inline comments.


================
Comment at: llvm/docs/LangRef.rst:11877
+'``llvm.swift.async.context.addr``' Intrinsic
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
----------------
I think the `^`s need to be the same length as the text above it, otherwise we'll get sphinx warnings.


================
Comment at: llvm/docs/LangRef.rst:11896
+
+If the function has a ``swiftasync`` parameter, that argument will initially
+be stored at the returned address. If not, it will be initialized to null.
----------------
by "the function" do you mean the caller?


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


================
Comment at: llvm/lib/Target/AArch64/AArch64ExpandPseudoInsts.cpp:706
+      .addUse(AArch64::X16)
+      .addImm(0xc31a)
+      .addImm(48)
----------------
The immediate probably needs an explanation. IIUC this is the discriminator?


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


================
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)
----------------
do you mean `#0x1000_1000_0000_0000`?


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


================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:2907
+
+    int FrameIdx;
+    unsigned Size = RegInfo->getSpillSize(*RC);
----------------
sink the declaration of `FrameIdx` to its definition.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:138
+  case AArch64::StoreSwiftAsyncContext:
+    NumBytes = 20;
+    break;
----------------
This is for that `pacdb` sequence right? Might be worth a comment here showing what the 5 instructions are.


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

https://reviews.llvm.org/D95044