[PATCH] D147584: [lld] Support separate minimized bitcode file path in --thinlto-prefix-replace

[Ivan Tadeu Ferreira Antunes Filho via Phabricator via llvm-commits]

itf added inline comments.


================
Comment at: lld/COFF/Config.h:228
   // thinLTOPrefixReplaceNativeObject, instead of thinLTOPrefixReplaceNew.
+  // If thinLTOPrefixReplaceMinimizedIndex, replace the prefix of minimized
+  // index files, thinLTOPrefixReplaceMinimizedIndex, with
----------------
tejohnson wrote:
> This should be thinLTOPrefixReplaceMinimizedBitcode not thinLTOPrefixReplaceMinimizedIndex to match the code (and MinimizedBitcode is a better name IMO).
> 
> I also think with this change that we should rename thinLTOPrefixReplaceOld to thinLTOPrefixReplaceFullBitcode, and thinLTOPrefixReplaceNew with thinLTOPrefixReplaceOutputFile or something similar, to clarify what each of these paths are.
> 
> Then I suggest making the comment something like "replace the prefix of the minimized thin link input bitcode files, thinLTOPrefixReplaceMinimizedBitcode, with thinLTOPrefixReplaceFullBitcode, when the bitcode path is written to output index and imports files, allowing..."
Changing the name of the variables makes it more clear. I replaced it here and on the other files


================
Comment at: lld/ELF/InputFiles.cpp:1586
     path = replaceThinLTOSuffix(mb.getBufferIdentifier());
+    if (!config->thinLTOPrefixReplaceMinimizedBitcode.empty()) {
+      path = lto::getThinLTOOutputFile(
----------------
tejohnson wrote:
> The way the code is written, both suffix and prefix replacing can be done. If we don't want people to rely on that, and want to eventually move off of suffix replacement, we probably should give an error somewhere, i.e. in the option parsing, if both are specified. Then maybe add a comment here that we have already checked that only one is in effect.
I think it is fine to have both suffix and prefix replace, as they don't interfere with one another. 

I can add a check so that this is not possible any longer and check the error in the tests, but if we intend to deprecate suffix replace this would just add more code to be removed in the future. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147584/new/

https://reviews.llvm.org/D147584