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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
