[PATCH] D78929: [AIX][XCOFF]emit extern linkage for the llvm intrinsic symbol

[Sean Fertile via Phabricator via llvm-commits]

sfertile added a subscriber: Xiangling_L.
sfertile added a comment.

👍 Thanks for trying this out Digger, I think this approach is the right one to take. I've had an initial look and left a few comments inline. I'll need to spend some time understanding ExternalSymbolSDNodes a bit better before being  able to review the patch thoroughly enough to approve.



================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:152
 private:
+  /// LLVM extern intrinsic xcoff symbols used for emitting the linkage of these
+  /// symbols.
----------------
jhenderson wrote:
> Should it be spelt XCOFF?
IIUC this isn't directly related to intrinsics, but to ExternalSymbolSD nodes, with the lowering of intrinsics being just one of the ways we can end up with an ExternalSymbolSDNodes. Intrinsic lowering might be the only way we expect to get one of these nodes on AIX right now but that can change in the future. The naming and comments need to reflect that this is explicitly for handling emitting the linkage of ExternalSymbolSDNodes.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1726
 
+void PPCAIXAsmPrinter::emitInstruction(const MachineInstr *MI) {
+  switch (MI->getOpcode()) {
----------------
Choosing to implement this override and handle here instead of adding to the base classes `emitInstruction` makes this really nice and clean. Good job.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1733
+  case PPC::BL8_NOP:
+  case PPC::BL_NOP:
+    const MachineOperand &MO = MI->getOperand(0);
----------------
What is the state of tail-calls on AIX? I know we can't tail call in many of the situations where we can on Linux, however if they aren't completely disabled we might need to add the tail-call opcodes here for similar transformation.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5283
+    if (Subtarget.isAIXABI()) {
+      // If there exists a user-declared function whose name is the same as the
+      // ExternalSymbol's, then we pick up the user-declared version.
----------------
I'm not too familiar with when we get ExternalSymbolSDNodes or what the semantic should be if we have an ExternalSymbolSDNode and a IR declaration for the same function, but it seems odd to treat the situation differently based on XCOFF vs ELF. @Xiangling_L Was `aix-user-defined-memcpy.ll` motivated by real code? If so I'd like to investigate it a bit more.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5294
+      auto &Context = DAG.getMachineFunction().getMMI().getContext();
+      const MCSymbolXCOFF *Sym = cast<MCSymbolXCOFF>(
+          Context.getOrCreateSymbol(Twine(".") + Twine(SymName)));
----------------
Why not leave the name as is, and prepend the '.' when we create/lookup the symbol in the AsmPrinter?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78929