[PATCH] D31027: [ThinLTO] Add support for emitting minimized bitcode for thin link

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: lib/Bitcode/Writer/BitcodeWriter.cpp:3786
+
+  // If a non-zero module hash was provided, use it instead of computing
+  // the hash.
----------------
I found it a little confusing that the behaviour is conditioned on ModHash having a specific value. I wonder whether something like this would be better?
```
if (GenerateHash) {
 // generate hash
 if (ModHash)
    // copy hash to ModHash
} else if (ModHash)
 // write out hash in ModHash
```


================
Comment at: lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:262
     // regular LTO module.
     WriteBitcodeToFile(&M, OS);
     return;
----------------
Do we also need to write the module to ThinLinkOS in this case?


================
Comment at: test/Transforms/ThinLTOBitcodeWriter/split.ll:23
+; RUN: llvm-bcanalyzer -dump %t4.thinlto.bc | grep -v "Summary of " >%t4.dump
+; RUN: diff %t3.dump %t4.dump
 
----------------
Can we not expect the file contents to be the same, and compare them directly? (same for other test cases)


================
Comment at: tools/llvm-lto/llvm-lto.cpp:120
 
+static cl::opt<std::string> ThinLTOObjectSuffixReplace(
+    "thinlto-object-suffix-replace",
----------------
Do you really need to add support for this feature to llvm-lto and llvm-lto2? It seems that this is just performing a sed operation on the file names. Can you achieve the same result by copying the `.thinlink.bc` files over the `.bc` files in your test cases?


https://reviews.llvm.org/D31027