<div dir="ltr">Initial, mostly writer-side, patch sent for review (D46699). PTAL.<div><br></div><div><div>A few misc changes from original format sent here:</div><div>- When a GV name is available (i.e. when printing the per-module summaries</div><div>  to assembly after the summary is built from the IR), instead of printing both</div><div>  the name and the GUID, which are redundant, print only the name and put the</div><div>  GUID in a comment after the summary entry. This should make it easier to write</div><div>  tests using summary assembly as input also, since the name can be used and the</div><div>  parser can compute the GUID on the fly.</div><div>- Support for printing of the type test related summary info, as discussed with</div><div>  Peter.</div><div>- Use "(" and ")" instead of curly braces, since the latter made it harder to</div><div>  test (FileCheck got confused when there were two curly braces in a row thinkig</div><div>  it was a regex).</div><div>- Suppression of printing of some less common flags/fields, when they hold their</div><div>  default 0 values.</div><div><br></div><div>Teresa</div><br><div class="gmail_quote"><div dir="ltr">On Fri, May 4, 2018 at 7:11 AM Teresa Johnson <<a href="mailto:tejohnson@google.com">tejohnson@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, May 3, 2018 at 10:13 PM David Blaikie via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, May 3, 2018 at 6:03 PM Mehdi AMINI <<a href="mailto:joker.eph@gmail.com" target="_blank">joker.eph@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div dir="ltr">Le jeu. 3 mai 2018 à 15:52, Peter Collingbourne <<a href="mailto:peter@pcc.me.uk" target="_blank">peter@pcc.me.uk</a>> a écrit :<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, May 3, 2018 at 3:29 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><span><div dir="ltr">On Thu, May 3, 2018 at 3:08 PM Peter Collingbourne via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, May 3, 2018 at 2:44 PM, Mehdi AMINI <span dir="ltr"><<a href="mailto:joker.eph@gmail.com" target="_blank">joker.eph@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><span><div dir="ltr">Le mar. 1 mai 2018 à 16:50, Teresa Johnson <<a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a>> a écrit :<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hi Mehdi, thanks for the comments, responses and a tweaked proposal below. Teresa<div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, May 1, 2018 at 11:37 AM, Mehdi AMINI <span dir="ltr"><<a href="mailto:joker.eph@gmail.com" target="_blank">joker.eph@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hi,<div><br></div><div>My main concern is this one:</div><span><div><br></div><div>> Currently, I am emitting the summary entries at the end, after the metadata nodes. Note that the ModuleSummaryIndex is not currently referenced from the Module, and isn’t currently created when parsing the Module IR bitcode (there is a separate derived class for reading the ModuleSummaryIndex from bitcode). This is because they are not currently used at the same time. However, in the future there is no reason why we couldn’t tag the global values in the Module’s LLVM assembly with the corresponding summary entry if the ModuleSummaryIndex is available when printing the Module in the assembly writer. I.e. we could do the following for “main” from the above example when printing the IR definition (note the “^3” at the end):</div><div><br></div><div><br></div></span><div>I believe the reason that the ModuleSummaryIndex is not attached to the Module is that it is fundamentally not a piece of IR, but it is conceptually really an Analysis result.</div><div>Usually other analyses don't serialize their result, we happen to serialize this one for an optimization purpose (reloading it and making the thin-link faster).</div></div></blockquote><div><br></div><div>True. My understanding is that the push for having it serialized via assembly is due to the fact that it is emitted into the bitcode. I know there is disagreement on this reasoning, I am hoping to have a proposal that is acceptable to everyone. =)</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>The fundamental problem is that an analysis result has to be able to be invalidated with IR changes, attaching this directly to the module wouldn't achieve this. The risk is that when the IR and the summary get out-of-sync (`clang -O2 my_module_with_summaries.ll -emit-llvm -o my_optimized module_with_summaries.ll`) the summaries would be badly wrong.</div><div><br></div><div>Have you looked into what it'd take to make it a "real" analysis in the pass manager?</div></div></blockquote><div><br></div><div>Thanks for raising this issue specifically, I hadn't addressed it in my proposal and it is a big one. I am not proposing that we attempt to maintain the summary through optimization passes, and definitely don't think we should do that. IMO deserializing it should be for testing the thin link and the combined summaries in the backends only. To that end, I have an idea (below some background first).</div><div><br></div><div>Note that in some cases the module summary analysis is an analysis pass. I.e. when invoked by "opt -module-summary=". However, some time ago when Peter added the support for splitting the bitcode (for CFI purposes) and therefore needed to generate a summary in each partition (Thin and Regular), he added the ThinLTOBitcodeWriterPass, which invokes the module summary builder directly (twice). This writer is what gets invoked now when building via "clang -flto=thin", and with "opt -thinlto-bc". So there it is not invoked/maintained as an analysis pass/result. It would be tricky to figure out how to even split rather than recompute the module summary index in that case. Even in the case where we are still invoking as an analysis pass (opt -module-summary), we would need to figure out how to read in the module summary to use as the analysis result when available (so that it could be invalidated and recomputed when stale).</div><div><br></div><div>Rather than add this plumbing, and just have it discarded if opt does any optimization, I think we should focus at least for the time being on supporting reading the summary from assembly exactly where we currently read in the summary from bitcode:</div><div>1) For the thin link (e.g. tools such as llvm-lto2 or llvm-lto, which currently have to be preceded by "opt -module-summary/-thinlto-bc" to generate an index, but could just build it from assembly instead).</div><div>2) For the LTO backends (e.g. tools such as llvm-lto which can consume a combined index and invoke the backends, or "clang -fthinlto-index=" for distributed ThinLTO backend testing), where we could build the combined summary index from assembly instead.</div><div><br></div><div>This greatly simplifies the reading side, as there are no optimizations performed on the IR after the index is read in these cases that would require invalidation. It also simplifies adding the parsing support, since it gets invoked exactly where we expect to build an index currently (i.e. since we don't currently build or store the ModuleSummaryIndex when parsing the Module from bitcode). It doesn't preclude someone from figuring out how to compute the module summary analysis result from the assembly, and invalidating it after optimization, when reading the Module IR via 'opt' in the future.</div><div><br></div><div>Does this seem like a reasonable proposal to everyone?</div></div></div></div></div></blockquote><div><br></div></span><div>Sounds good to me.</div><div><br></div><div>That would make .ll files quite convenient during debugging I think? We could disassemble, manually change summaries, and re-assemble a bitcode file before running the (thin-)link again.<br></div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Yes, that seems more reasonable than what I thought you had in mind. If the only consumer of this information is llvm-as, then the purpose of the asm summary format is just to provide a way to create a .bc file for testing purposes, which is certainly a useful capability.</div></div></div></div></blockquote></span><div><br>That feels a bit surprising if ".ll -> llvm-as -> .bc -> <some other tool - opt, etc>" is different from ".ll -> <opt, etc>". Is that what we're talking about here? Any chance that can be avoided & feeding a .ll file works (in the sense of does the same thing/tests the same behavior) in all the same places that feeding a .bc file does? (as is the case with non-summary-based IR, to the best of my knowledge)<br></div></div></div></blockquote><div><br></div><div>I may be mistaken, but I don't think we have a lot of tools that can read both .ll and .bc and end up using the summary if it is a .bc file. LTO can't read .ll, for example. The only one that I can think of is clang and presumably we could make that use whichever API we would use in llvm-as for reading the summary from .ll. So the behaviour of most tools would be that ".ll -> llvm-as -> .bc -> <some tool>" vs ".ll -> <some tool>" would end up being the same in both cases: the summary gets discarded.</div></div></div></div></blockquote><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div>There is still a discrepancy in that:</div></div></div><div dir="ltr"><div class="gmail_quote"><div><br></div><div> .ll -> llvm-as -> .bc<br></div><div><br></div></div></div><div dir="ltr"><div class="gmail_quote"><div>would be different from:</div><div><br></div><div> .ll -> opt -> .bc<br></div><div><br></div><div>The latter would drop the summary.</div></div></div></blockquote><div><br>Though by the sounds of it .bc -> opt -> .bc drops the summary already? So there's some consistency there.<br><br>But, yeah, it's not clear to me if it'd be better if that did something else. This is mostly/purely for testing/development purposes anyway.<span id="gmail-m_3546174527598727259m_-5652756522620902288m_-3144670029993966461gmail-m_-6377244508080862267m_5004165039369931912m_6710373724474087095m_4269345380332382296m_8921911820903467664m_7729192863744272269m_9036781251802416550m_7418588459110944206m_4502817952738636235gmail-m_-6956089728037873786m_4795617359147801528gmail-m_-8353203249030920200m_6567874537655844917m_5112085973537410555gmail-docs-internal-guid-737316de-f81b-6dbe-8709-b6a8255a9c91"><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt;display:inline"><span style="font-size:11pt;font-family:Arial;color:rgb(0,0,0);background-color:transparent;font-variant-east-asian:normal;vertical-align:baseline;white-space:pre-wrap">view these records is via “llvm-bcanalyzer -dump”, then manually decoding the raw bitcode dumps.</span></p></span></div></div></div></blockquote><div><br></div><div>Yep, I sent a similar response - we're all crossing wires here because the long emails keep getting held for moderator approval. =)  Trimmed a bunch of stuff so this one should go through.</div><div><br></div><div>It sounds like we have a pretty good consensus at this point. Unless there is an objection, I'll start preparing patches.<br></div><div><br></div><div>Thanks,</div><div>Teresa</div></div><div><br></div>-- <br><div dir="ltr" class="gmail-m_3546174527598727259gmail_signature"><span style="font-family:Times;font-size:medium"><table cellspacing="0" cellpadding="0"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td style="border-top:2px solid rgb(213,15,37)">Teresa Johnson |</td><td style="border-top:2px solid rgb(51,105,232)"> Software Engineer |</td><td style="border-top:2px solid rgb(0,153,57)"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td style="border-top:2px solid rgb(238,178,17)"> 408-460-2413</td></tr></tbody></table></span></div></div></blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><span style="font-family:Times;font-size:medium"><table cellspacing="0" cellpadding="0"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td style="border-top:2px solid rgb(213,15,37)">Teresa Johnson |</td><td style="border-top:2px solid rgb(51,105,232)"> Software Engineer |</td><td style="border-top:2px solid rgb(0,153,57)"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td style="border-top:2px solid rgb(238,178,17)"> 408-460-2413</td></tr></tbody></table></span></div></div></div>