[PATCH] D98183: [libLTO] Add support for -save-temps.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/tools/llvm-lto2/llvm-lto2.cpp:67
+namespace llvm {
+extern cl::opt<bool> SaveTemps;
+}
----------------
qiongsiwu1 wrote:
> @tejohnson I am going back to this comment https://reviews.llvm.org/D153559#4441538 indicating that it may not be ideal to use the `SaveTemps` option defined in `LTOCodeGenerator.cpp` here for `llvm-lto2`. This particular change was made because we cannot have two `save-temps` coexisting. I thought about renaming one, or both options, but renaming could make both options confusing. Should I call the `llvm-lto2` option `lto2-save-temps`? That is probably not ideal. 
> 
> May I get some advice? Is it ok if we reuse the same option? Thanks in advance! 
I wouldn't want to rename the existing flag in llvm-lto2, since a ton of tests already use it.

It looks like the convention in LTOCodeGenerator.cpp is to preface options with "lto-", so perhaps that one should be -lto-save-temps. That would also be more consistent with the -thinlto-save-temps option used for ThinLTOCodeGenerator.cpp (defined in llvm-lto.cpp).

While it could work to use the same option, it seems a bit odd to share in this case. I think it only works in this case because llvm-lto2 depends on the LTO library which currently includes the code for both LTO APIs, even though llvm-lto2 doesn't use the legacy one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98183