[PATCH] D47905: [ThinLTO] Parse module summary index from assembly
Duncan P. N. Exon Smith via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jun 20 09:38:29 PDT 2018
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
More information about the llvm-commits
mailing list