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

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.

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?



================
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
 
----------------
Why did you switch from llvm-as to opt in these tests?


================
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,
----------------
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.


================
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 {
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D47905