<div dir="ltr">+1 to what Justin said here. Both the namespace trick and the private members being questionable.<br></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Feb 23, 2016 at 3:19 PM Justin Bogner via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Owen Anderson <<a href="mailto:resistor@mac.com" target="_blank">resistor@mac.com</a>> writes:<br>
> resistor created this revision.<br>
> resistor added reviewers: chandlerc, bogner.<br>
> resistor added a subscriber: llvm-commits.<br>
> resistor set the repository for this revision to rL LLVM.<br>
><br>
> Because the new pass manager exposes the pass types, member functions<br>
> on those passes become exported symbols by default.  We can use<br>
> LLVM_LIBRARY_VISIBILITY to restrict their linkage to be accessible<br>
> within the LLVM dynamic library itself, but not exported from it.<br>
<br>
Hiding the symbols from the "private" namespace certainly makes sense,<br>
but I'm not completely sold that hiding the private members really gains<br>
us much. More thoughts below.<br>
<br>
> Repository:<br>
>   rL LLVM<br>
><br>
> <a href="http://reviews.llvm.org/D17347" rel="noreferrer" target="_blank">http://reviews.llvm.org/D17347</a><br>
><br>
> Files:<br>
>   include/llvm/Transforms/Scalar/SROA.h<br>
>   lib/Transforms/Scalar/SROA.cpp<br>
><br>
> Index: lib/Transforms/Scalar/SROA.cpp<br>
> ===================================================================<br>
> --- lib/Transforms/Scalar/SROA.cpp<br>
> +++ lib/Transforms/Scalar/SROA.cpp<br>
> @@ -206,7 +206,7 @@<br>
>  /// for the slices used and we reflect that in this structure. The uses are<br>
>  /// stored, sorted by increasing beginning offset and with unsplittable slices<br>
>  /// starting at a particular offset before splittable slices.<br>
> -class llvm::sroa::AllocaSlices {<br>
> +class LLVM_LIBRARY_VISIBILITY llvm::sroa::AllocaSlices  {<br>
>  public:<br>
>    /// \brief Construct the slices of a particular alloca.<br>
>    AllocaSlices(const DataLayout &DL, AllocaInst &AI);<br>
> @@ -4274,7 +4274,7 @@<br>
>    static char ID;<br>
>  };<br>
><br>
> -char SROALegacyPass::ID = 0;<br>
> +char LLVM_LIBRARY_VISIBILITY SROALegacyPass::ID = 0;<br>
<br>
It's probably simpler/clearer to handle AllocaSlices and<br>
SROALegacyPass::ID by setting LLVM_LIBRARY_VISIBILITY on the internal<br>
namespace. Ie, in SROA.h we'd just say:<br>
<br>
  namespace sroa LLVM_LIBRARY_VISIBILITY {<br>
  class AllocaSliceRewriter;<br>
  class AllocaSlices;<br>
  class Partition;<br>
  class SROALegacyPass;<br>
  }<br>
<br>
However we do this though, this part is obvious and LGTM. Hiding the<br>
symbols that are only visible to work around having to support both pass<br>
managers is pure goodness.<br>
<br>
><br>
>  FunctionPass *llvm::createSROAPass() { return new SROALegacyPass(); }<br>
><br>
> Index: include/llvm/Transforms/Scalar/SROA.h<br>
> ===================================================================<br>
> --- include/llvm/Transforms/Scalar/SROA.h<br>
> +++ include/llvm/Transforms/Scalar/SROA.h<br>
> @@ -112,16 +112,19 @@<br>
><br>
>    /// Helper used by both the public run method and by the legacy pass.<br>
>    PreservedAnalyses runImpl(Function &F, DominatorTree &RunDT,<br>
> -                            AssumptionCache &RunAC);<br>
> +                            AssumptionCache &RunAC) LLVM_LIBRARY_VISIBILITY;<br>
><br>
> -  bool presplitLoadsAndStores(AllocaInst &AI, sroa::AllocaSlices &AS);<br>
> +  bool presplitLoadsAndStores(AllocaInst &AI, sroa::AllocaSlices &AS)<br>
> +         LLVM_LIBRARY_VISIBILITY;<br>
>    AllocaInst *rewritePartition(AllocaInst &AI, sroa::AllocaSlices &AS,<br>
> -                               sroa::Partition &P);<br>
> -  bool splitAlloca(AllocaInst &AI, sroa::AllocaSlices &AS);<br>
> -  bool runOnAlloca(AllocaInst &AI);<br>
> -  void clobberUse(Use &U);<br>
> -  void deleteDeadInstructions(SmallPtrSetImpl<AllocaInst *> &DeletedAllocas);<br>
> -  bool promoteAllocas(Function &F);<br>
> +                               sroa::Partition &P) LLVM_LIBRARY_VISIBILITY;<br>
> +  bool splitAlloca(AllocaInst &AI, sroa::AllocaSlices &AS)<br>
> +         LLVM_LIBRARY_VISIBILITY;<br>
> +  bool runOnAlloca(AllocaInst &AI) LLVM_LIBRARY_VISIBILITY;<br>
> +  void clobberUse(Use &U) LLVM_LIBRARY_VISIBILITY;<br>
> +  void deleteDeadInstructions(SmallPtrSetImpl<AllocaInst *> &DeletedAllocas)<br>
> +         LLVM_LIBRARY_VISIBILITY;<br>
> +  bool promoteAllocas(Function &F) LLVM_LIBRARY_VISIBILITY;<br>
<br>
This, on the other hand, seems like a fair bit of boilerplate for not<br>
much gain. These are private methods and are used by run(), so hiding<br>
them won't really help us dead strip more or anything like that, and I<br>
don't really see why passes should be special compared to other LLVM<br>
classes in this regard.<br>
<br>
The way I see it, if it's worth hiding the private members here, why<br>
wouldn't we do something to generally hide private members in most or<br>
all of LLVM?<br>
<br>
>  };<br>
><br>
>  }<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>