[PATCH] D127778: [LTO][ELF] Add selective --save-temps= option

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: lld/ELF/Driver.cpp:1170
+  } else {
+    for (auto &s : args.getAllArgValues(OPT_save_temps_eq)) {
+      auto it = llvm::find(saveTempsValues, s);
----------------
Use `args.filtered(OPT_save_temps_eq)` which avoids a temporary std::vector. Then you can just use the value since its backing memory has sufficient lifetime. And you can avoid `adl_end` below.


================
Comment at: lld/ELF/Driver.cpp:1172
+      auto it = llvm::find(saveTempsValues, s);
+      if (it != llvm::adl_end(saveTempsValues))
+        config->saveTempsArgs.insert(llvm::StringRef(*it));
----------------
`std::end`


================
Comment at: lld/ELF/Driver.cpp:1175
+      else
+        error("unknown --save-temps: " + s);
+    }
----------------



================
Comment at: lld/test/ELF/lto/save-temps-eq.ll:19
+; RUN: ld.lld main.o thin1.o --save-temps=preopt --save-temps --save-temps=\opt -o %t/all2/a.out
+; RUN: diff %t/all2/a.out %t/all/a.out
+; RUN: mv *.o.* %t/all2
----------------
Prefer `cmp` for binary comparison.


================
Comment at: llvm/include/llvm/LTO/Config.h:275
+                     bool UseInputModulePath = false,
+                     const llvm::DenseSet<llvm::StringRef> &SaveTempsArgs = {});
 };
----------------
Omit llvm:: prefix since the delcaration is in `namespace llvm { ... }`


================
Comment at: llvm/lib/LTO/LTOBackend.cpp:86
+    std::string OutputFileName, bool UseInputModulePath,
+    const llvm::DenseSet<llvm::StringRef> &SaveTempsArgs) {
   ShouldDiscardValueNames = false;
----------------
omit llvm:: prefix


================
Comment at: llvm/test/ThinLTO/X86/selective-save-temps.ll:5
+
+; Copy IR from import-constant.ll since it generates all the temps
+; RUN: opt -thinlto-bc %s -o 1.bc
----------------
If you use `;;` for non-RUN non-CHECK lines, change this `;` too. Be consistent.


================
Comment at: llvm/test/ThinLTO/X86/selective-save-temps.ll:14
+; RUN:    -import-constants-with-refs \
+; RUN:    -r=1.bc,main,plx \
+; RUN:    -r=1.bc,_Z6getObjv,l \
----------------
-r per line may be too verbose. You can pack multiple `-r` on one line.


================
Comment at: llvm/tools/llvm-lto2/llvm-lto2.cpp:75
+                   ",combinedindex"),
+    cl::desc("Save select temporary files. Cannot be specified together with "
+             "-save-temps"),
----------------
selected


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127778