[PATCH] D128771: [ThinLTO][test] Add tests for emitting files in-process

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Thanks! Couple minor comments.



================
Comment at: llvm/test/ThinLTO/X86/emit-inprocess-files.ll:10
+; RUN: opt -thinlto-bc %p/Inputs/distributed_import.ll -thin-link-bitcode-file=%t2.thinlink.bc -o %t2.bc
+; RUN: llvm-bcanalyzer -dump %t1.thinlink.bc | FileCheck --check-prefix=THINLINKBITCODE %s
+
----------------
Feel free to remove this if we are already testing the minimized bitcode option elsewhere.


================
Comment at: llvm/tools/llvm-lto2/llvm-lto2.cpp:85
+                                "InProcessThinLTO. Must be specified together "
+                                "with -thinlto-emit-indexes"));
+
----------------
As suggested below, we should check this for the distributed indexes case too. Also, either change the wording to something like "Has no effect unless specified with ...", or do some error checking.


================
Comment at: llvm/tools/llvm-lto2/llvm-lto2.cpp:315
                                             /* NewPrefix */ "",
                                             /* ShouldEmitImportsFiles */ true,
                                             /* LinkedObjectsFile */ nullptr,
----------------
For completeness, this should also use the new ThinLTOEmitImports. But then you will unfortunately need to update a number of existing tests (hopefully not too many as we don't tend to look at the contents or for the existence of the imports files though - a quick search suggests maybe only llvm/test/ThinLTO/X86/emit_imports.ll).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128771