Keep Optimization Remark Yaml in NewPM

[lenary]

Summary

The New Pass Manager infrastructure was forgetting to keep around the optimization remark yaml file that the compiler might have been producing. This meant setting the option to '-' for stdout worked, but setting it to a filename didn't give file output (presumably it was deleted because compilation didn't explicitly keep it). This change just ensures that the file is kept if compilation succeeds.

So far I have updated one of the optimization remark output tests to add a version with the new pass manager. It is my intention for this patch to also include changes to all tests that use -opt-remark-output= but I wanted to get the code patch ready for review while I was making all those changes.

Fixes https://bugs.llvm.org/show_bug.cgi?id=33951

[anemet]

LGTM, thanks!

tools/opt/NewPMDriver.cpp

175

I know that I called this YamlFile in opt.cpp but that's really unfortunate in retrospect. Can you please call this something like OptRemarksFile or something?

[lenary]

Better Patch for opt remark file issue, undoing changes to new PM interface, and renaming variables as requested.

[chandlerc]

tools/opt/opt.cpp

553–554

For Out and ThinLinkOut, we do the ->keep() call inside runPassPipeline. I would keep this consistent with the other files.

[lenary]

Move OptRemarkFile back into runPassPipeline

[lenary]

All remark tests now go through both the old and new pass managers.

[chandlerc]

LGTM. Andrew already said LGTM, so feel free to land.

test/Transforms/GVN/opt-remarks.ll

 ; RUN: opt < %s -gvn -o /dev/null  -pass-remarks-output=%t -S -pass-remarks=gvn \
 ; RUN:     2>&1 | FileCheck %s
 ; RUN: cat %t | FileCheck -check-prefix=YAML %s
+; RUN: opt < %s -aa-pipeline=basic-aa -passes=gvn -o /dev/null -pass-remarks-output=%t -S -pass-remarks=gvn \
+; RUN:     2>&1 | FileCheck %s
+; RUN: cat %t | FileCheck -check-prefix=YAML %s
 
 ; CHECK:      remark: <unknown>:0:0: load of type i32 eliminated{{$}}
 ; CHECK-NEXT: remark: <unknown>:0:0: load of type i32 eliminated{{$}}
 ; CHECK-NEXT: remark: <unknown>:0:0: load of type i32 eliminated{{$}}
 ; CHECK-NOT:  remark:
 
 ; YAML:      --- !Passed
 ; YAML-NEXT: Pass:            gvn

test/Transforms/LoopVectorize/X86/vectorization-remarks-missed.ll

 ; RUN: opt < %s -loop-vectorize -S -pass-remarks-missed='loop-vectorize' -pass-remarks-analysis='loop-vectorize' 2>&1 | FileCheck %s
 ; RUN: opt < %s -loop-vectorize -o /dev/null -pass-remarks-output=%t.yaml
 ; RUN: cat %t.yaml | FileCheck -check-prefix=YAML %s
 
+; RUN: opt < %s -passes=loop-vectorize -S -pass-remarks-missed='loop-vectorize' -pass-remarks-analysis='loop-vectorize' 2>&1 | FileCheck %s
+; RUN: opt < %s -passes=loop-vectorize -o /dev/null -pass-remarks-output=%t.yaml
+; RUN: cat %t.yaml | FileCheck -check-prefix=YAML %s
+
 ; C/C++ code for tests
 ; void test(int *A, int Length) {
 ; #pragma clang loop vectorize(enable) interleave(enable)
 ;   for (int i = 0; i < Length; i++) {
 ;     A[i] = i;
 ;     if (A[i] > Length)
 ;       break;
 ;   }

test/Transforms/NewGVN/opt-remarks.ll

 ; XFAIL: *
 ; RUN: opt < %s -newgvn -o /dev/null  -pass-remarks-output=%t -S -pass-remarks=gvn \
 ; RUN:     2>&1 | FileCheck %s
 ; RUN: cat %t | FileCheck -check-prefix=YAML %s
+; RUN: opt < %s -passes=newgvn -o /dev/null  -pass-remarks-output=%t -S -pass-remarks=gvn \
+; RUN:     2>&1 | FileCheck %s
+; RUN: cat %t | FileCheck -check-prefix=YAML %s
+
 
 ; CHECK:      remark: <unknown>:0:0: load of type i32 eliminated{{$}}
 ; CHECK-NEXT: remark: <unknown>:0:0: load of type i32 eliminated{{$}}
 ; CHECK-NEXT: remark: <unknown>:0:0: load of type i32 eliminated{{$}}
 ; CHECK-NOT:  remark:
 
 ; YAML:      --- !Passed
 ; YAML-NEXT: Pass:            gvn

test/Transforms/PGOProfile/memop_size_opt.ll

+; RUN: opt < %s -pgo-memop-opt -pgo-memop-count-threshold=90 -pgo-memop-percent-threshold=15 -S | FileCheck %s --check-prefix=MEMOP_OPT
 ; RUN: opt < %s -passes=pgo-memop-opt -pgo-memop-count-threshold=90 -pgo-memop-percent-threshold=15 -S | FileCheck %s --check-prefix=MEMOP_OPT
 ; RUN: opt < %s -pgo-memop-opt -pgo-memop-count-threshold=90 -pgo-memop-percent-threshold=15 -pass-remarks-with-hotness -pass-remarks-output=%t.opt.yaml -S | FileCheck %s --check-prefix=MEMOP_OPT
 ; RUN: FileCheck %s -input-file=%t.opt.yaml --check-prefix=YAML
+; RUN: opt < %s -passes=pgo-memop-opt -pgo-memop-count-threshold=90 -pgo-memop-percent-threshold=15 -pass-remarks-with-hotness -pass-remarks-output=%t.opt.yaml -S | FileCheck %s --check-prefix=MEMOP_OPT
+; RUN: FileCheck %s -input-file=%t.opt.yaml --check-prefix=YAML
+
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 define void @foo(i8* %dst, i8* %src, i8* %dst2, i8* %src2, i32* %a, i32 %n) !prof !27 {
 entry:
   br label %for.cond
 

test/Transforms/SLPVectorizer/AArch64/getelementptr.ll

 ; RUN: opt -S -slp-vectorizer -slp-threshold=-18 -dce -instcombine -pass-remarks-output=%t < %s | FileCheck %s
 ; RUN: cat %t | FileCheck -check-prefix=YAML %s
+; RUN: opt -S -passes='slp-vectorizer,dce,instcombine' -slp-threshold=-18 -pass-remarks-output=%t < %s | FileCheck %s
+; RUN: cat %t | FileCheck -check-prefix=YAML %s
+
 
 target datalayout = "e-m:e-i32:64-i128:128-n32:64-S128"
 target triple = "aarch64--linux-gnu"
 
 ; These tests check that we remove from consideration pairs of seed
 ; getelementptrs when they are known to have a constant difference. Such pairs
 ; are likely not good candidates for vectorization since one can be computed
 ; from the other. We use an unprofitable threshold to force vectorization.

test/Transforms/SLPVectorizer/AArch64/horizontal.ll

 ; RUN: opt -slp-vectorizer -slp-threshold=-6 -S -pass-remarks-output=%t <  %s | FileCheck %s
 ; RUN: cat %t | FileCheck -check-prefix=YAML %s
+; RUN: opt -passes=slp-vectorizer -slp-threshold=-6 -S -pass-remarks-output=%t <  %s | FileCheck %s
+; RUN: cat %t | FileCheck -check-prefix=YAML %s
+
 
 ; FIXME: The threshold is changed to keep this test case a bit smaller.
 ; The AArch64 cost model should not give such high costs to select statements.
 
 target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
 target triple = "aarch64--linux"
 
 ; CHECK-LABEL: test_select

test/Transforms/SampleProfile/remarks.ll

 ; RUN: opt < %s -sample-profile -sample-profile-file=%S/Inputs/remarks.prof -S -pass-remarks=sample-profile -pass-remarks-output=%t.opt.yaml 2>&1 | FileCheck %s
-; RUN: opt < %s -passes=sample-profile -sample-profile-file=%S/Inputs/remarks.prof -S -pass-remarks=sample-profile 2>&1 | FileCheck %s
 ; RUN: FileCheck %s -check-prefix=YAML < %t.opt.yaml
+; RUN: opt < %s -passes=sample-profile -sample-profile-file=%S/Inputs/remarks.prof -S -pass-remarks=sample-profile -pass-remarks-output=%t.opt.yaml 2>&1 | FileCheck %s
+; RUN: FileCheck %s -check-prefix=YAML < %t.opt.yaml
+
 ; Original test case.
 ;
 ;     1    #include <stdlib.h>
 ;     2
 ;     3    long long foo() {
 ;     4      long long int sum = 0;
 ;     5      for (int i = 0; i < 500000000; i++)
 ;     6        if (i < 1000)

test/Transforms/Util/libcalls-opt-remarks.ll

-; RUN: opt < %s -instcombine -o /dev/null  -pass-remarks-output=%t -S -pass-remarks=instcombine \
-; RUN:     2>&1 | FileCheck %s
+; RUN: opt < %s -instcombine -o /dev/null  -pass-remarks-output=%t -S \
+; RUN:     -pass-remarks=instcombine 2>&1 | FileCheck %s
+; RUN: cat %t | FileCheck -check-prefix=YAML %s
+; RUN: opt < %s -passes='require<opt-remark-emit>,instcombine' -o /dev/null \
+; RUN:     -pass-remarks-output=%t -S -pass-remarks=instcombine 2>&1 | FileCheck %s
 ; RUN: cat %t | FileCheck -check-prefix=YAML %s
-; RUN: opt < %s -passes='require<opt-remark-emit>,instcombine' -o /dev/null -S -pass-remarks=instcombine \
-; RUN:     2>&1 | FileCheck %s
 
 ; CHECK:      remark: libcalls-opt-remarks.c:10:10: folded strlen(select) to select of constants{{$}}
 ; CHECK-NOT:  remark:
 
 ; YAML:      --- !Passed
 ; YAML-NEXT: Pass:            instcombine
 ; YAML-NEXT: Name:            simplify-libcalls
 ; YAML-NEXT: DebugLoc:        { File: libcalls-opt-remarks.c, Line: 10, Column: 10 }

tools/opt/NewPMDriver.h

 /// inclusion of the new pass manager headers and the old headers into the same
 /// file. It's interface is consequentially somewhat ad-hoc, but will go away
 /// when the transition finishes.
 ///
 /// ThinLTOLinkOut is only used when OK is OK_OutputThinLTOBitcode, and can be
 /// nullptr.
 bool runPassPipeline(StringRef Arg0, Module &M, TargetMachine *TM,
                      tool_output_file *Out, tool_output_file *ThinLinkOut,
-                     StringRef PassPipeline, opt_tool::OutputKind OK,
-                     opt_tool::VerifierKind VK,
+                     tool_output_file *OptRemarkFile, StringRef PassPipeline,
+                     opt_tool::OutputKind OK, opt_tool::VerifierKind VK,
                      bool ShouldPreserveAssemblyUseListOrder,
                      bool ShouldPreserveBitcodeUseListOrder,
                      bool EmitSummaryIndex, bool EmitModuleHash);
 }
 
 #endif

tools/opt/NewPMDriver.cpp

 namespace polly {
 void RegisterPollyPasses(PassBuilder &);
 }
 #endif
 
 bool llvm::runPassPipeline(StringRef Arg0, Module &M, TargetMachine *TM,
                            tool_output_file *Out,
                            tool_output_file *ThinLTOLinkOut,
+                           tool_output_file *OptRemarkFile,
                            StringRef PassPipeline, OutputKind OK,
                            VerifierKind VK,
                            bool ShouldPreserveAssemblyUseListOrder,
                            bool ShouldPreserveBitcodeUseListOrder,
                            bool EmitSummaryIndex, bool EmitModuleHash) {
   bool VerifyEachPass = VK == VK_VerifyEachPass;
 
   Optional<PGOOptions> P;
   switch (PGOKindFlag) {
     case InstrGen:
#endif
   MPM.run(M, MAM);
 
   // Declare success.
   if (OK != OK_NoOutput) {
     Out->keep();
     if (OK == OK_OutputThinLTOBitcode && ThinLTOLinkOut)
       ThinLTOLinkOut->keep();
   }
+
+  if (OptRemarkFile)
+    OptRemarkFile->keep();
+
   return true;
 }

tools/opt/opt.cpp

if (!DisableDITypeMap)
     Context.enableDebugTypeODRUniquing();
 
   if (PassRemarksWithHotness)
     Context.setDiagnosticsHotnessRequested(true);
 
   if (PassRemarksHotnessThreshold)
     Context.setDiagnosticsHotnessThreshold(PassRemarksHotnessThreshold);
 
-  std::unique_ptr<tool_output_file> YamlFile;
+  std::unique_ptr<tool_output_file> OptRemarkFile;
   if (RemarksFilename != "") {
     std::error_code EC;
-    YamlFile = llvm::make_unique<tool_output_file>(RemarksFilename, EC,
+    OptRemarkFile = llvm::make_unique<tool_output_file>(RemarksFilename, EC,
                                                         sys::fs::F_None);
     if (EC) {
       errs() << EC.message() << '\n';
       return 1;
     }
     Context.setDiagnosticsOutputFile(
-        llvm::make_unique<yaml::Output>(YamlFile->os()));
+        llvm::make_unique<yaml::Output>(OptRemarkFile->os()));
   }
 
   // Load the input module...
   std::unique_ptr<Module> M = parseIRFile(InputFilename, Err, Context);
 
   if (!M) {
     Err.print(argv[0], errs());
     return 1;
if (NoVerify)
       VK = VK_NoVerifier;
     else if (VerifyEach)
       VK = VK_VerifyEachPass;
 
     // The user has asked to use the new pass manager and provided a pipeline
     // string. Hand off the rest of the functionality to the new code for that
     // layer.
     return runPassPipeline(argv[0], *M, TM.get(), Out.get(), ThinLinkOut.get(),
-                           PassPipeline, OK, VK, PreserveAssemblyUseListOrder,
+                           OptRemarkFile.get(), PassPipeline, OK, VK,
+                           PreserveAssemblyUseListOrder,
                            PreserveBitcodeUseListOrder, EmitSummaryIndex,
                            EmitModuleHash)
                ? 0
                : 1;
   }
 
   // Create a PassManager to hold and optimize the collection of passes we are
   // about to build.
   //
   legacy::PassManager Passes;
 
   // Add an appropriate TargetLibraryInfo pass for the module's triple.
   TargetLibraryInfoImpl TLII(ModuleTriple);
 
   // The -disable-simplify-libcalls flag actually disables all builtin optzns.
   if (DisableSimplifyLibCalls)
     TLII.disableAllFunctions();
   Passes.add(new TargetLibraryInfoWrapperPass(TLII));
if (Buffer.size() != CompileTwiceBuffer.size() ||
         (memcmp(Buffer.data(), CompileTwiceBuffer.data(), Buffer.size()) !=
          0)) {
       errs() << "Running the pass manager twice changed the output.\n"
                 "Writing the result of the second run to the specified output.\n"
                 "To generate the one-run comparison binary, just run without\n"
                 "the compile-twice option\n";
       Out->os() << BOS->str();
       Out->keep();
-      if (YamlFile)
-        YamlFile->keep();
+      if (OptRemarkFile)
+        OptRemarkFile->keep();
       return 1;
     }
     Out->os() << BOS->str();
   }
 
   // Declare success.
   if (!NoOutput || PrintBreakpoints)
     Out->keep();
 
-  if (YamlFile)
-    YamlFile->keep();
+  if (OptRemarkFile)
+    OptRemarkFile->keep();
 
   if (ThinLinkOut)
     ThinLinkOut->keep();
 
   return 0;
 }