[PATCH] D76932: [AIX] emit .extern and .weak directive linkage

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1496
+
+    MCSymbol *Name = getSymbol(&F);
+    if (TM.getTargetTriple().isOSBinFormatXCOFF() && !F.isIntrinsic()) {
----------------
DiggerLin wrote:
> jasonliu wrote:
> > This block of code and D78045 will have conflict. One of us will need to rebase.
> the one who land later will rebase.
My understanding is that this would need a semantic reconciliation. I'd like to see the rebase of this patch before approving. Also, this would be another place to look into for the follow-up mentioned in https://reviews.llvm.org/D78045?id=257331#inline-714634 @jasonliu.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:445
+    }
+    llvm_unreachable("Should never emit this");
   case GlobalValue::AppendingLinkage:
----------------
We can fall through using an annotation to suppress the warning:
```
LLVM_FALLTHROUGH;
```


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1505
+
+      MCSectionXCOFF *Csect = cast<MCSectionXCOFF>(
+          getObjFileLowering().getSectionForExternalReference(&F, TM));
----------------
A comment here to explain that we are emitting linkage for the function descriptor would help.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:2014
   case GlobalValue::LinkOnceODRLinkage:
+  case GlobalValue::LinkOnceAnyLinkage:
+  case GlobalValue::WeakAnyLinkage:
----------------
Swap the order of the two `LinkOnce*` cases to match their definition order.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:2021
         "There is no mapping that implements AppendingLinkage for XCOFF.");
   default:
     report_fatal_error(
----------------
Since `AvailableExternallyLinkage` is the only value left, just replace `default` with that and change the `report_fatal_error` to say `AvailableExternallyLinkage` for its first instance of "linkage".


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1641
 
-  // External global variables are already handled.
-  if (GV->isDeclaration())
+  if (GV->isDeclaration()) {
+    emitLinkage(GV, Csect->getQualNameSymbol());
----------------
This should probably be `isDeclarationForLinker`. It seems we need a larger followup for `AvailableExternallyLinkage` that would involve checking our calls to `isDeclaration`:

```
define void @_Z1gv() {
entry:
  call void @_Z1fIiEvv()
  ret void
}

; Function Attrs: inlinehint nounwind
define available_externally void @_Z1fIiEvv() #0 {
entry:
  ret void
}

attributes #0 = { inlinehint nounwind }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76932