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

[Digger via Phabricator via cfe-commits]

DiggerLin marked 16 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1500
+      if (cast<MCSymbolXCOFF>(Name)->hasContainingCsect())
+        emitLinkage(&F, Name);
+
----------------
jasonliu wrote:
> 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. 
when we implement .bl foo to .bl foo[PR]
the SymbolName will change from .bl[SMC] and check the .bl[SMC]->hasRepresentedCsectSet()


================
Comment at: llvm/test/CodeGen/PowerPC/aix-WeakODRLinkage.ll:10
+  ret void
+}
+
----------------
jasonliu wrote:
> Do we also want to test WeakAnyLinkage?
WeakAnyLinkage should be weak
It has been tested in aix-weak.ll

define weak void @foo_weak(i32* %p)  {
entry:
  %p.addr = alloca i32*, align 4
  store i32* %p, i32** %p.addr, align 4
  %0 = load i32*, i32** %p.addr, align 4
  %1 = load i32, i32* %0, align 4
  %inc = add nsw i32 %1, 1
  store i32 %inc, i32* %0, align 4
  ret void
} 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76932