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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson 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.
----------------
pcc wrote:
> 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
> ```
I like the idea of changing GenerateHash to guard just the computation of a new hash value. Will do.


================
Comment at: lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp:262
     // regular LTO module.
     WriteBitcodeToFile(&M, OS);
     return;
----------------
pcc wrote:
> Do we also need to write the module to ThinLinkOS in this case?
That's a good question. I think it is simplest to emit it to the ThinLinkOS as well, will add that. In this case there is no summary index, and the thin link will produce a native .o file. But for simplicity in the build system where we want to be able to know what output files to expect, and how to feed those into the thin link action, yes, it would be best to emit that.


================
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
 
----------------
pcc wrote:
> Can we not expect the file contents to be the same, and compare them directly? (same for other test cases)
True, will do that instead.


================
Comment at: tools/llvm-lto/llvm-lto.cpp:120
 
+static cl::opt<std::string> ThinLTOObjectSuffixReplace(
+    "thinlto-object-suffix-replace",
----------------
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?


https://reviews.llvm.org/D31027