[PATCH] D73817: Add PassManagerImpl.h to hide implementation details

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D73817#1858364 <https://reviews.llvm.org/D73817#1858364>, @rnk wrote:

> In D73817#1855896 <https://reviews.llvm.org/D73817#1855896>, @dblaikie wrote:
>
> > But there's an explicit instantiation declaration, right? Which should prevent any implicit instantiation.
>
>
> I think Clang will instantiate the template anyway to make an available_externally definition for optimization purposes.


My understanding is that it's not allowed to do this if the inline keyword is not present (which it doesn't appear to be in this case on any of the functions... oh, right, because they're defined inline so they're implicitly 'inline').

>>> As for why it's expensive, there are two DenseMaps in AnalysisManager, and this function inserts into both maps. In -ftime-trace, those instantiations happen to be attributed to getResultImpl. It's possible that some of those DenseMapPair & std::pair instantiations happen later anyway after this change, but you can see the memory usage reduction is real.
>> 
>> My understanding is that the explicit instantiation declaration should actually make it invalid for the C++ compiler to instantiate the function at all, except where it sees the explicit instantiation definition.
>> 
>> For instance this example:
>> 
>> template<typename T>
>>  void f1() {
>> 
>>   typename T::u v;
>> 
>> }
>>  extern template void f1<int>();
>>  int main() {
>> 
>>   f1<int>();
>> 
>> }
>> 
>> Successfully compiles because the compiler doesn't instantiate the contents of f1<int> due to the presence of the explicit instantiation declaration. If you add the "inline" keyword to the f1 declaration, then the compiler is allowed to instantiate f1<int> in this TU and it does reject it due to "T::u" being invalid for T=int.
> 
> That's interesting. Clang produces the error if you put the function in a class template:
> 
>   template <typename T> struct Foo {
>     static void f1() { typename T::u v; }
>   };
>   extern template struct Foo<int>;
>   int main() { Foo<int>::f1(); }

Ah, right - I missed the implicit inline in class members.

> It also errors if you add `inline`:
> 
>   template <typename T> inline void f1() { typename T::u v; }
>   extern template void f1<int>();
>   int main() { f1<int>(); }
> 
> 
> I think this proves the theory about available_externally definitions. In other words, extern template isn't as useful as we think it is.

Except it can only do this if they're 'inline' (implicitly or explicitly), and with optimizations enabled. But if there's no particular benefit to inlining these, I can see the desire to disable that even in optimized builds by having the explicit instantiation.

With that in mind, I believe you could get the same functionality with your patch without splitting into a separate file that would need to be #included in certain places, by putting those definitions outside the class definition (as they are in the separate file) after the class definition in the same file.

This would make it harder to break the build (I believe this change broke some polly compilation - at least I couldn't figure out how to fix it & eventually removed polly from my LLVM_ENABLE_PROJECTS build) by keeping the header standalone/implicitly instantiable, but still getting the compile time benefits of the explicit instantiations


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73817