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

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:152
 private:
+  /// LLVM extern intrinsic xcoff symbols used for emitting the linkage of these
+  /// symbols.
----------------
sfertile wrote:
> 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.
Sorry I wasn't more explicit in my previous comment. I don't think we should be using the name 'intrinsic' here at all. 

```
/// Symbols lowered from ExternalSymbolSDNodes.
SmallPtrSet<MCSymbol *, 8> ExternalSymbols;
```


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1734
+  case PPC::BL_NOP:
+  case PPC::BL_TLS:
+  case PPC::BL8_TLS:
----------------
We haven't implemented TLS on AIX yet, and I'm not sure which if any of these call pseudos will be used. How about adding them  as a separate case with a report_fatal_error which can be revisited as part of TLS implementation on AIX.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5264
 
+  const auto getAIXIntrinsicFuncExternalSymbolSDNode =
+      [&](const char *SymName) {
----------------
I'm don't think this is what Jason meant from his comment.


================
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)));
----------------
DiggerLin wrote:
> sfertile wrote:
> > Why not leave the name as is, and prepend the '.' when we create/lookup the symbol in the AsmPrinter?
> if leave name as is, the branch will be
> bl memset  
> not 
> bl .memset
> 
> if leave name as is , we want to generate bl .memset . we need to modify the operand(ExternalSymbol) of branch to generate correct bl .memset
Yes, which is why I said you would move where you prepend the '.' from here, to where you create the MCSymbol from the MO_ExternalSymbol in the ASM printer. 

I think our handling of when there is deceleration in the IR matching the ExtrernalSymbol name is wrong, and it should either be done for all subtargets, or none (but I'm not 100% sure of that yet and investigating). If they are to be common-ed, then by  moving modifying the symbols name too the ASMprinter, it lets us have identical code for all subtargets for transforming ExternalSymbolSDNode callees. 

I'd say hold off on making this change until we better understand the IR Decl/ExternalSym situation though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78929