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

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/include/llvm/MC/MCContext.h:105
+    /// linkage.
+    std::vector<MCSymbol *> XCOFFIntrinsicSymbols;
+    using XCOFFIntrinsicSym_iterator = std::vector<MCSymbol *>::iterator;
----------------
Just a general comment on the data structure that's using here.
Have we thought about using using a SmallPtrSet so that you don't need to worry about inserting the duplicated value into the vector?


================
Comment at: llvm/include/llvm/MC/MCContext.h:423
+    /// Add a XCOFF Intrinsic entern symbol into XCOFFIntrinsicSymbols.
+    void addXCOFFIntrinsicExternSymbol(MCSymbol *Sym);
+
----------------
Is it necessary to have wrappers to the real data structure? 
I feel like returning the actual data structure here is not that bad. And user of this data structure have much more freedom using something like range-based for loop, instead of the iterator interface.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-llvm-intrinsic.ll:19
+
+; Function Attrs: noinline nounwind optnone
+define void @bar() {
----------------
nit: remove this Attr.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78929