[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/Transforms/IPO/ThinLTOBitcodeWriter.cpp:470
+llvm::createWriteThinLTOBitcodePass(raw_ostream &Str,
+                                    std::string ThinLinkBitcodeFile) {
+  return new WriteThinLTOBitcode(Str, ThinLinkBitcodeFile);
----------------
It's a little odd that this takes a stream for one output and a path for the other. I think I would prefer both to be streams so that the client can handle any errors opening the thin link bitcode file.


================
Comment at: tools/llvm-lto/llvm-lto.cpp:120
 
+static cl::opt<std::string> ThinLTOObjectSuffixReplace(
+    "thinlto-object-suffix-replace",
----------------
tejohnson wrote:
> pcc wrote:
> > 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?
> Technically it isn't needed here, but I do need it in gold-plugin, and I was trying to mirror that in these tools. It is being tested in the new gold test I added, so maybe not. Should I go ahead and remove this from llvm-lto and llvm-lto2?
I'd prefer to remove them. The tests aren't really testing anything other than the code in the testing tools, as opposed to the gold plugin tests which are presumably testing production code. If the distributed build functionality were a feature of `lib/LTO` we could test it via llvm-lto2, and from that perspective it could make some sense to leave the test code in llvm-lto2 only for a possible refactoring.


https://reviews.llvm.org/D31027