[PATCH] D46699: [ThinLTO] Print module summary index to assembly

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked an inline comment as done.
tejohnson added a comment.

In https://reviews.llvm.org/D46699#1111535, @pcc wrote:

> This is somewhat of a bikeshed comment but I'm not really a big fan of putting everything on one line. (I know we do that for debug metadata too, but really I'd be in favour of changing that as well.) I'm wondering whether something like this would be easier to read:
>
>   ^9 = gv: ( ; guid = 17381606045411660303
>     name: "hot_function",
>     summaries: (
>       function: (
>         module: ^0,
>         flags: (
>           linkage: external,
>           notEligibleToImport: 0,
>           live: 0,
>           dsoLocal: 0
>         ),
>         insts: 16,
>         calls: (
>           (callee: ^5, hotness: hot),
>           (callee: ^6, hotness: cold),
>           (callee: ^4, hotness: hot),
>           (callee: ^7, hotness: cold),
>           (callee: ^10, hotness: none),
>           (callee: ^3, hotness: hot),
>           (callee: ^2, hotness: none),
>           (callee: ^8, hotness: none),
>           (callee: ^1, hotness: critical)
>         )
>       )
>     )
>   )
>


Thanks for the comments.

I see pros and cons. In some ways this is nicer to look at, but I am concerned about the amount of vertical space the summary assembly will consume. In most cases I would like to see more summary entries on my screen at a time, but that is probably personal preference.

Incidentally, I confirmed that my WIP parser can actually handle this (Duncan pointed out and I confirmed BTW that multi-line metadata are supported by the parser, although I don't believe they are emitted that way). Although the support in this patch to skip the summary assembly will not handle this, as it only skips a line at a time and won't understand the subsequent lines which don't start with the summary ID.



================
Comment at: docs/LangRef.rst:5721
+Specifically, for the Thin Link portion of a ThinLTO compile (either
+via the linker or tools such as llvm-lto2), or when parsing a combined index
+for a distributed ThinLTO backend via clang's "``-fthinlto-index=<>``" flag.
----------------
pcc wrote:
> Probably wouldn't mention the linker, see this part of the thread with David: http://lists.llvm.org/pipermail/llvm-dev/2018-May/122975.html
Forgot about that, fixed.


================
Comment at: lib/IR/AsmWriter.cpp:2776
+static std::string getLinkagePrintName(GlobalValue::LinkageTypes LT,
+                                       bool AddSpace = true) {
+  std::string Space = AddSpace ? " " : "";
----------------
pcc wrote:
> Maybe changing the other callers to do:
> ```if (GV->getLinkage() != GlobalValue::ExternalLinkage)
>    Out << getLinkagePrintName(GV->getLinkage()) << ' ';
> ```
> would be clearer than adding the argument?
I thought about that but felt it was less churn and clearer to handle it in one centralized place. If you much prefer changing the callers, I can do that instead.


Repository:
  rL LLVM

https://reviews.llvm.org/D46699