[PATCH] D157547: Arm64EC entry/exit thunks, consolidated.

[Daniel Paoliello via Phabricator via cfe-commits]

dpaoliello added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2726-2729
+        // FIXME: For non-dllimport functions, MSVC emits the same entry
+        // twice, for reasons I don't understand.  I have to assume the linker
+        // ignores the redundant entry; there aren't any reasonable semantics
+        // to attach to it.
----------------
To be clear: you're seeing MSVC emit the same entry twice, but you opted to emit the entry only once?


================
Comment at: llvm/lib/Target/AArch64/AArch64Arm64ECCallLowering.cpp:332-333
+  }
+  // FIXME: Copy anything other than sret?  Shouldn't be necessary for normal
+  // C ABI, but might show up in other cases.
+  BasicBlock *BB = BasicBlock::Create(M->getContext(), "", F);
----------------
Having a look through the available parameter attributes:
* Do we need `byval` and `byref`, or are they irrelevant to the thunk?
* Should we copy the alignment attributes to keep them the same?
* Might be interesting to copy some of the optimization related ones (unless we're too late in the compilation) like `readnone`, `readonly`, `immarg`, `returned`, etc.


================
Comment at: llvm/lib/Target/AArch64/AArch64Arm64ECCallLowering.cpp:608
+  //
+  // FIXME: Handle functions with weak linkage?
+  if (F.hasExternalLinkage() || F.hasWeakLinkage() || F.hasLinkOnceLinkage()) {
----------------
Are you asking if we should be handling functions with weak linkage, or is this a TODO to implement support somewhere else?


================
Comment at: llvm/lib/Target/AArch64/AArch64Arm64ECCallLowering.cpp:641-645
+      // FIXME: If a function is dllimport, we can just mark up the symbol
+      // using hybmp$x, and everything just works.  If the function is not
+      // marked dllimport, we can still mark up the symbol, but we somehow
+      // need an extra stub to compute the correct callee. Not really
+      // understanding how this works.
----------------
Can you provide some more details about this? What extra stub are you seeing?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:1618
+  if (Subtarget->isWindowsArm64EC()) {
+    // FIXME: are there other intrinsics we need to add here?
+    setLibcallName(RTLIB::MEMCPY, "#memcpy");
----------------
Full list is:
```
#ceil
#floor
#memcmp
#memcpy
#memmove
#memset
```


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.td:4616-4617
   def SEH_PACSignLR : Pseudo<(outs), (ins), []>, Sched<[]>;
+  def SEH_SaveAnyRegQP : Pseudo<(outs), (ins i32imm:$reg0, i32imm:$reg1, i32imm:$offs), []>, Sched<[]>;
+  def SEH_SaveAnyRegQPX : Pseudo<(outs), (ins i32imm:$reg0, i32imm:$reg1, i32imm:$offs), []>, Sched<[]>;
 }
----------------
Should these be added to `AArch64InstrInfo::isSEHInstruction`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157547