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

[Jason Liu via Phabricator via cfe-commits]

jasonliu added a comment.

Thanks for splitting the test case. I think it helps a lot for reading and maintainability.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1490
       continue;
     GlobalValue::VisibilityTypes V = F.getVisibility();
+
----------------
This line is not used before XCOFF specialized code, we should move it down. 


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1495
+      if (F.isIntrinsic())
+        continue;
+
----------------
If I understand correctly, we do not want to touch how we interact with visibility in this patch. 
If that's the case, we don't want to `continue` here and in line 1512 since the early `continue` will change the code flow for visibility. 
Suggestion:
if (TM.getTargetTriple().isOSBinFormatXCOFF() && !F.isIntrinsic()) {
and remove continue in line 1512. 


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1498
+      // Get the function entry point symbol.
+      Name = OutContext.getOrCreateSymbol("." + Name->getName());
+      if (cast<MCSymbolXCOFF>(Name)->hasContainingCsect())
----------------
nit: I don't think it's a good idea here to assign back to `Name` here, as `Name` will also get used in  `emitVisibility` below and we want to keep it as it is. Let's create a new MCSymbol* here.  


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1500
+      if (cast<MCSymbolXCOFF>(Name)->hasContainingCsect())
+        emitLinkage(&F, Name);
+
----------------
1. We need to rebase here, as it is called `hasRepresentedCsectSet()` instead of `hasContainingCsect()` now.
2. I'm slightly worried here to rely on `hasRepresentedCsectSet()` to check if a linkage should be emitted. This is based on the assumption that we will not ever change our implementation for `.bl foo` to `.bl foo[PR]`. But in https://reviews.llvm.org/D77080#inline-706207, we discussed about this possibility. So this assumption might not be true in the future. However, I'm not sure if there is another way to check if this function have been called directly. 
So if there is another way to check, we should pursue the alternative instead. If there is not, then we need to add an assert here, like `assert(Name->getName().equals(cast<MCSymbolXCOFF>(Name)->getUnqualifiedName())` to make sure we don't get a qualname here. 
3. 
Have a comment here and tell people what we are doing here.
For example, 
// If there is a direct call to external function, then we need to emit linkage for its function entry point. 


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1503
+      if (F.hasAddressTaken()) {
+        // Indirect call reference of the extern function.
+        // for example c source code as :
----------------
Comment need to mention what we are trying to do here: 
// If address is taken from an extern function, we need to emit linkage for its function descriptor symbol.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1504
+        // Indirect call reference of the extern function.
+        // for example c source code as :
+        // extern int bar_ext();
----------------
nit: for -> For,
and we don't need space between `as` and  `:`


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1510
+        Name = Csect->getQualNameSymbol();
+        emitLinkage(&F, Name);
+      }
----------------
nit: We don't need to assign it back to `Name`.
emitLinkage(&F, Csect->getQualNameSymbol());


================
Comment at: llvm/test/CodeGen/PowerPC/aix-WeakODRLinkage.ll:10
+  ret void
+}
+
----------------
Do we also want to test WeakAnyLinkage?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76932