[PATCH] D40508: Replace long type names in IR with hashes

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff marked 3 inline comments as done.
sepavloff added a comment.

In https://reviews.llvm.org/D40508#936617, @rnk wrote:

> It's not clear to me that this abbreviation functionality should live in Support. Clang probably wants enough control over this (assuming we're doing it) that the logic should live in clang.
>
> I also think we might want to try solving this a completely different way: just don't bother emitting more than two template arguments for IR type names. We don't really need to worry about type name uniqueness or matching them across TUs.


Actually the intention is to have unique type names across different TUs.
I will publish the relevant patch a bit latter, but the problem we are solving is in incorrect behavior of `llvm-link`. If one TU contains opaque type and the other has the type definition, these two types must be merged into one. The merge procedure relies on type names, so it is important to have the same type names for types in different TUs that are equivalent in the sense of C++.



================
Comment at: include/clang/AST/PrettyPrinter.h:231
+  /// make things like breaking digraphs and '>>' in template parameters.
+  bool NotForCompilation : 1;
 };
----------------
rjmccall wrote:
> This saves, what, a few spaces from a few thousand IR type names?  I'm skeptical that this even offsets the code-size impact of adding this option.
Not these few spaces themselves make the issue. The real evil is that to insert these spaces, `printTemplateArgumentList` had to print each template parameter into intermediate stream. We could try to use `raw_abbrev_ostream` to reduce memory consumption, it would not work because these intermediate streams are of type `raw_svector_ostream` and all these huge parameter texts would be materialized first and only then would go to compacting stream.
If no need to maintain compilable output, intermediate streams are not needed and all input can go directly to `raw_abbrev_ostream`.


================
Comment at: lib/AST/TypePrinter.cpp:1532-1534
+namespace {
+template<typename TA>
+void printTo(raw_ostream &OS, ArrayRef<TA> Args, const PrintingPolicy &Policy,
----------------
rnk wrote:
> `static` is nicer than a short anonymous namespace.
Yes, but this is function template. It won't create symbol in object file. Actually anonymous namespace has no effect here, it is only a documentation hint.


https://reviews.llvm.org/D40508