[PATCH] D47905: [ThinLTO] Parse module summary index from assembly

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D47905#1137869, @dexonsmith wrote:

> I just have a couple of additional (minor) comments on the code, although I admit it was hard to be thorough as a it's a monolithic patch.  I might have preferred if this were committed incrementally, adding support for 1-2 constructs at a time.  Because the lexing/parsing code is fairly mechanical, I think splitting it up now would be quite likely to //introduce// errors so I'm not going to ask for that.
>
> In terms of testing, I don't see any //positive// assembly parsing lit tests (aside from implicit ones when round-tripping bitcode through `llvm-as`), and I think that would be valuable.  I'm not sure if it's pragmatic to write targeted tests for each construct, or if a couple of monolithic tests would be better.  What do you think?


Thanks for the review! Agree it is a rather large patch - sorry. I wanted to implement it fully before sending for review because I was concerned there would be too much churn as I encountered issues while building various parts of the index from assembly.

As a follow on I was planning on adding support to tools like llvm-lto/llvm-lto2 to parse the index from assembly for simulating the thin link and will add more tests reading from assembly then. But I agree this patch should have some for completeness. How about for now a monolithic test that tries to cover all the various summary constructs in assembly, then runs through llvm-as | llvm-dis to ensure we get the input back out as expected?



================
Comment at: include/llvm/AsmParser/Parser.h:87
+/// \param DataLayoutString Override datalayout in the llvm assembly.
+std::pair<std::unique_ptr<Module>, std::unique_ptr<ModuleSummaryIndex>>
+parseAssemblyFileWithIndex(StringRef Filename, SMDiagnostic &Error,
----------------
dexonsmith wrote:
> I wonder if it's worth adding a simple named struct for this.
Sure I can do that.


================
Comment at: include/llvm/AsmParser/Parser.h:136
+
+/// parseSummaryIndexAssemblyFile is a wrapper around this function.
+/// Parse LLVM Assembly for summary index from a MemoryBuffer.
----------------
dexonsmith wrote:
> I feel like this comment should come after you describe what the function does.  I think the next line should be first (and have an empty line after, so that it's the only thing showing up in doxygen "brief").
Sure, will fix and the other one I added like this above (this was a clone/modify of the comment for parseAssembly)


================
Comment at: include/llvm/IR/ModuleSummaryIndex.h:775-776
+  // don't have the string owned elsewhere (e.g. the Strtab on a module).
+  StringSaver Saver;
+  BumpPtrAllocator Alloc;
+
----------------
dexonsmith wrote:
> The comment suggests these aren't always used.  Should they be in an optional?
Looks like I forgot to create the patch as a diff based on a couple other patches I pulled out of this big one. I'll see if I can fix the patch to rebase on top of those. Note this code in particular I pulled out into a separate patch D47842.

Regarding your suggestion, I'm not sure we can use Optional here since StringSaver is non-copyable.

(I also pulled out some other functionality into D47844).


================
Comment at: test/Assembler/thinlto-bad-summary1.ll:3
 ; summary type label.
-; RUN: not llvm-as %s 2>&1 | FileCheck %s
+; RUN: not opt %s 2>&1 | FileCheck %s
 
----------------
dexonsmith wrote:
> Why did you switch from llvm-as to opt in these tests?
Because these tests are specifically testing the functionality added in the assembly writing patch to skip module entries (LLParser::SkipModuleSummaryEntry). With this patch, that is no longer invoked by llvm-as, but it will continue to be invoked by opt, i.e. via the existing parseAssembly handling (in the RFC we agreed it is better to ignore the summary when optimizing the IR, and only use it in limited situations where we will be testing e.g. the thin link).


================
Comment at: tools/llvm-as/llvm-as.cpp:97
+    // the expected behavior.
+    if ((M && (!M->empty() || !M->global_empty())) || !IndexToWrite)
+      WriteBitcodeToFile(*M, Out->os(), PreserveBitcodeUseListOrder,
----------------
dexonsmith wrote:
> I think putting `IndexToWrite` first would make the condition easier for me to parse.  It might even design away the need for the assertion that follows in the `else` block.
Makes sense, will change (and agree the assertion can be removed).


================
Comment at: tools/llvm-as/llvm-as.cpp:100-102
+    // Otherwise, with an empty Module but non-empty Index, we write a
+    // combined index.
+    else {
----------------
dexonsmith wrote:
> I personally don't love the style of dangling the `else` after a comment that's outdented from the "then" statement.  Reading the code sequentially, I get tricked into thinking the `if` is complete, and then the `else` is a big surprise when I see it.  I'd prefer:
> ```
> if (cond) {
>   // We have a non-empty module: write it plus the ...
>   ...
> } else {
>   // We have an empty module but non-empty index: write a ...
>   ...
> }
> ```
> However, if you think it's clearer/better as is, that's fine.
Sure, I will fix that.


Repository:
  rL LLVM

https://reviews.llvm.org/D47905