[PATCH] D127168: [llvm-reduce] Add support for LTO bitcode files

[Matthew Voss via Phabricator via llvm-commits]

ormris added inline comments.


================
Comment at: tools/llvm-reduce/ReducerWorkItem.cpp:425
+    if (std::error_code EC = MB.getError()) {
+      WithColor::error(errs(), ToolName) << Filename << ": " << EC.message() << "\n";
       return std::unique_ptr<ReducerWorkItem>();
----------------
arsenm wrote:
> Single quotes around newline
Fixed.


================
Comment at: tools/llvm-reduce/ReducerWorkItem.cpp:434
+        Err.print(ToolName, errs());
+        return std::unique_ptr<ReducerWorkItem>();
+      }
----------------
arsenm wrote:
> Why is this returning a default constructed ReducerWorkItem instead of null? I see some other places are doing this but I don't see how this makes sense
I opted for this to stay consistent with the surrounding code. Nonetheless, `nullptr` is more efficient at the same task, so I'll go ahead and use that instead.


================
Comment at: tools/llvm-reduce/ReducerWorkItem.cpp:441
+      if (MMM->LTOInfo->IsThinLTO && MMM->LTOInfo->EnableSplitLTOUnit)
+       initializeTargetInfo();
+    }
----------------
arsenm wrote:
> Why does an IR reduction need to initialize target info?
The ThinLTOBitcodeWriter pass depends on target info. It crashes without it.


================
Comment at: tools/llvm-reduce/ReducerWorkItem.cpp:455
   auto CloneMMM = std::make_unique<ReducerWorkItem>();
-  if (TM) {
+  if (MMM.MMI) {
     // We're assuming the Module IR contents are always unchanged by MIR
----------------
arsenm wrote:
> Why this change?
That change is no longer needed. I'll remove it.


================
Comment at: tools/llvm-reduce/llvm-reduce.cpp:121
+    legacy::PassManager PM;
+    PM.add(llvm::createWriteThinLTOBitcodePass(OutStream));
+    PM.run(*(M.M));
----------------
arsenm wrote:
> Do you really need a pass manager (a legacy one at that) just to write out the file? Why isn't there a simple function to use?
Unfortunately, there isn't. If you look at `opt`, you'll see the same strategy.


================
Comment at: tools/llvm-reduce/llvm-reduce.cpp:137
+  if (!IF) {
+    WithColor::error(errs()) << IF.takeError();
+    exit(1);
----------------
arsenm wrote:
> Other places propagate the tool name from argv
Fixed. I've added the tool name to the TestRunner class, so that it's accessible. 


================
Comment at: tools/llvm-reduce/llvm-reduce.cpp:144
+  if (!LI || !MOrErr) {
+    WithColor::error(errs()) << IF.takeError();
+    exit(1);
----------------
arsenm wrote:
> Ditto
See above.


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

https://reviews.llvm.org/D127168