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

[Digger via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/MC/MCContext.h:105
+    /// linkage.
+    std::vector<MCSymbol *> XCOFFIntrinsicSymbols;
+    using XCOFFIntrinsicSym_iterator = std::vector<MCSymbol *>::iterator;
----------------
hubert.reinterpretcast wrote:
> jasonliu wrote:
> > DiggerLin wrote:
> > > jasonliu wrote:
> > > > 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?
> > > /// SmallPtrSet - This class implements a set which is optimized for holding
> > >  /// SmallSize or less elements.  This internally rounds up SmallSize to the next
> > >  /// power of two if it is not already a power of two.  See the comments above
> > >  /// SmallPtrSetImplBase for details of the algorithm.
> > >  template<class PtrType, unsigned SmallSize>
> > >  class SmallPtrSet : public SmallPtrSetImpl<PtrType> {
> > >    // In small mode SmallPtrSet uses linear search for the elements, so it is
> > >    // not a good idea to choose this value too high. You may consider using a
> > >    // DenseSet<> instead if you expect many elements in the set.
> > >    static_assert(SmallSize <= 32, "SmallSize should be small");
> > > 
> > > There are hundreds llvm intrinsic function. I will use std::set   
> > There are hundreds of llvm intrinsic functions. But are you expecting every compilation will contain hundreds of llvm intrinsic functions? I would say most of the time, you would see those less than 8intrinsic functions in one compilation unit , except for some very heavy mathematic case. 
> I agree with @jasonliu, and even if `SmallPtrSet` is too small, why not use `DenseSet` like the comments say?
I think we can not guarantee the llvm intrinsic sysbols in a compilation unit is less than 32. I will  use DenseSet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78929