[MachineFunctionPass] Support -filter-passes for -print-changed

[MaskRay]

Summary

[MachineFunctionPass] Support -filter-passes for -print-changed

-filter-passes specifies a PassID (a lower-case dashed-separated pass name,
also used by -print-after, -stop-after, etc) instead of a CamelCasePass.

-filter-passes=CamelCaseNewPMPass seems like a workaround for new PM passes before
we can use lower-case dashed-separated pass names (as used by -passes=).

Example:

# getPassName() is "IRTranslator". PassID is "irtranslator"
llc -mtriple=aarch64 -print-changed -filter-passes=irtranslator < print-changed-machine.ll

Close https://github.com/llvm/llvm-project/issues/57453

[MaskRay]

remove unneeded comments
fix test

[rahmanl]

Thanks for implementing this. Why should there be a discrepancy in how passes are named with this option vs. the IR mode option? Wouldn't that make it hard for the user invoking clang with -mllvm?

[MaskRay]

In D133055#3762484, @rahmanl wrote:

Thanks for implementing this. Why should there be a discrepancy in how passes are named with this option vs. the IR mode option? Wouldn't that make it hard for the user invoking clang with -mllvm?

The lower-case dash-separated passs id is desired, as implemented in this patch. This matches -print-after=, -stop-after=, etc.

-filter-passes=CamelCasePass is a workaround for new PM passes before there is a better way.

[jamieschmeiser]

llvm/lib/Passes/StandardInstrumentations.cpp

64

I don't think you meant to remove this comment...

[aeubanks]

lg with comments resolved

llvm/lib/CodeGen/MachineFunctionPass.cpp

155

isFunctionInPrintList should also use "filtered out"

llvm/test/Other/print-changed-machine.ll

37

perhaps a QUIET-FILTER-NOT: legalizer?

[MaskRay]

add back two comments (but simplified)

llvm/lib/CodeGen/MachineFunctionPass.cpp

155

There is a difference between MachineFunctionPass and a new PM pass filter: MachineFunctionPass does not print anything for a -filter-print-funcs ignored function while a new PM pass filter prints " filtered out".

Personally I feel the MachineFunctionPass behavior is more reasonable but I will note that there is inconsistency. At any rate this difference probably should be addressed in this patch.

llvm/test/Other/print-changed-machine.ll

37

--implicit-check-not='IR Dump' covers this check :)

[aeubanks]

llvm/lib/Passes/StandardInstrumentations.cpp

64

comment is still removed

[MaskRay]

llvm/lib/Passes/StandardInstrumentations.cpp

64

Looks like the comment is duplicated.

// An option that supports the -print-changed option.  See
// the description for -print-changed for an explanation of the use
// of this option.  Note that this option has no effect without -print-changed.

I have kept one copy.

[nikic]

FYI there was some small but measurable compile-time impact, probably because you are looking up the pass info unconditionally, even if no printing is requested. (http://llvm-compile-time-tracker.com/compare.php?from=c0433f91d3333d8902ae7a53c0e5ee5e98b7586b&to=8d95fd7e56bed7d3a3260bed7117023968f8be3c&stat=instructions)

[aeubanks]

In D133055#3766237, @nikic wrote:

FYI there was some small but measurable compile-time impact, probably because you are looking up the pass info unconditionally, even if no printing is requested. (http://llvm-compile-time-tracker.com/compare.php?from=c0433f91d3333d8902ae7a53c0e5ee5e98b7586b&to=8d95fd7e56bed7d3a3260bed7117023968f8be3c&stat=instructions)

hopefully fixed with 7e0a52e8e9ef6394bb62e0b56e17fa23e7262411

[nikic]

In D133055#3776285, @aeubanks wrote:

In D133055#3766237, @nikic wrote:

FYI there was some small but measurable compile-time impact, probably because you are looking up the pass info unconditionally, even if no printing is requested. (http://llvm-compile-time-tracker.com/compare.php?from=c0433f91d3333d8902ae7a53c0e5ee5e98b7586b&to=8d95fd7e56bed7d3a3260bed7117023968f8be3c&stat=instructions)

hopefully fixed with 7e0a52e8e9ef6394bb62e0b56e17fa23e7262411

Thanks! Unfortunately the build was broken when this landed, so the results also include many other commits, but it does look good :) http://llvm-compile-time-tracker.com/compare.php?from=178554f3c8f983bd192818b6d46e4d95560c4964&to=fd2475049e882e6c70a745cbe0799749ba184910&stat=instructions

llvm/include/llvm/IR/PrintPasses.h

 // The list of passes to print before/after, if we only want to print
 // before/after specific passes.
 std::vector<std::string> printBeforePasses();
 std::vector<std::string> printAfterPasses();
 
 // Returns true if we should always print the entire module.
 bool forcePrintModuleIR();
 
+// Return true if -filter-passes is empty or contains the pass name.
+bool isPassInPrintList(StringRef PassName);
+
 // Returns true if we should print the function.
 bool isFunctionInPrintList(StringRef FunctionName);
 
 // Perform a system based diff between \p Before and \p After, using \p
 // OldLineFormat, \p NewLineFormat, and \p UnchangedLineFormat to control the
 // formatting of the output. Return an error message for any failures instead
 // of the diff.
 std::string doSystemDiff(StringRef Before, StringRef After,
                          StringRef OldLineFormat, StringRef NewLineFormat,
                          StringRef UnchangedLineFormat);
 
 } // namespace llvm
 
 #endif // LLVM_IR_PRINTPASSES_H

llvm/lib/CodeGen/MachineFunctionPass.cpp

#endif
 
   // If we want size remarks, collect the number of MachineInstrs in our
   // MachineFunction before the pass runs.
   if (ShouldEmitSizeRemarks)
     CountBefore = MF.getInstructionCount();
 
   // For --print-changed, if the function name is a candidate, save the
   // serialized MF to be compared later.
-  // TODO Implement --filter-passes.
   SmallString<0> BeforeStr, AfterStr;
-  bool ShouldPrintChanged = PrintChanged != ChangePrinter::None &&
+  StringRef PassID;
+  if (const PassInfo *PI = Pass::lookupPassInfo(getPassID()))
+    PassID = PI->getPassArgument();
+  const bool IsInterestingPass = isPassInPrintList(PassID);
+  const bool ShouldPrintChanged = PrintChanged != ChangePrinter::None &&
+                                  IsInterestingPass &&
                                   isFunctionInPrintList(MF.getName());
   if (ShouldPrintChanged) {
     raw_svector_ostream OS(BeforeStr);
     MF.print(OS);
   }
 
   bool RV = runOnMachineFunction(MF);
 
   if (ShouldEmitSizeRemarks) {
if (ShouldEmitSizeRemarks) {
     }
   }
 
   MFProps.set(SetProperties);
   MFProps.reset(ClearedProperties);
 
   // For --print-changed, print if the serialized MF has changed. Modes other
   // than quiet/verbose are unimplemented and treated the same as 'quiet'.
+  if (ShouldPrintChanged || !IsInterestingPass) {
     if (ShouldPrintChanged) {
       raw_svector_ostream OS(AfterStr);
       MF.print(OS);
-    if (BeforeStr != AfterStr) {
-      StringRef Arg;
-      if (const PassInfo *PI = Pass::lookupPassInfo(getPassID()))
-        Arg = PI->getPassArgument();
-      errs() << ("*** IR Dump After " + getPassName() + " (" + Arg + ") on " +
-                 MF.getName() + " ***\n");
+    }
+    if (IsInterestingPass && BeforeStr != AfterStr) {
+      errs() << ("*** IR Dump After " + getPassName() + " (" + PassID +
+                 ") on " + MF.getName() + " ***\n");
       switch (PrintChanged) {
       case ChangePrinter::None:
         llvm_unreachable("");
       case ChangePrinter::Quiet:
       case ChangePrinter::Verbose:
       case ChangePrinter::DotCfgQuiet:   // unimplemented
       case ChangePrinter::DotCfgVerbose: // unimplemented
         errs() << AfterStr;
if (IsInterestingPass && BeforeStr != AfterStr) {
         errs() << doSystemDiff(BeforeStr, AfterStr, Removed, Added, NoChange);
         break;
       }
       }
     } else if (llvm::is_contained({ChangePrinter::Verbose,
                                    ChangePrinter::DiffVerbose,
                                    ChangePrinter::ColourDiffVerbose},
                                   PrintChanged.getValue())) {
-      errs() << ("*** IR Dump After " + getPassName() + " on " + MF.getName() +
-                 " omitted because no change ***\n");
+      const char *Reason =
+          IsInterestingPass ? " omitted because no change" : " filtered out";
+      errs() << "*** IR Dump After " << getPassName();
+      if (!PassID.empty())
+        errs() << " (" << PassID << ")";
+      errs() << " on " << MF.getName() + Reason + " ***\n";
     }
   }
   return RV;
 }
 
 void MachineFunctionPass::getAnalysisUsage(AnalysisUsage &AU) const {
   AU.addRequired<MachineModuleInfoWrapperPass>();
   AU.addPreserved<MachineModuleInfoWrapperPass>();

llvm/lib/IR/PrintPasses.cpp

DiffBinary("print-changed-diff-path", cl::Hidden, cl::init("diff"),
                cl::desc("system diff used by change reporters"));
 
 static cl::opt<bool>
     PrintModuleScope("print-module-scope",
                      cl::desc("When printing IR for print-[before|after]{-all} "
                               "always print a module IR"),
                      cl::init(false), cl::Hidden);
 
+// See the description for -print-changed for an explanation of the use
+// of this option.
+static cl::list<std::string> FilterPasses(
+    "filter-passes", cl::value_desc("pass names"),
+    cl::desc("Only consider IR changes for passes whose names "
+             "match the specified value. No-op without -print-changed"),
+    cl::CommaSeparated, cl::Hidden);
+
 static cl::list<std::string>
     PrintFuncsList("filter-print-funcs", cl::value_desc("function names"),
                    cl::desc("Only print IR for functions whose name "
                             "match this for all print-[before|after][-all] "
                             "options"),
                    cl::CommaSeparated, cl::Hidden);
 
 /// This is a helper to determine whether to print IR before or

 }
 
 std::vector<std::string> llvm::printAfterPasses() {
   return std::vector<std::string>(PrintAfter);
 }
 
 bool llvm::forcePrintModuleIR() { return PrintModuleScope; }
 
+bool llvm::isPassInPrintList(StringRef PassName) {
+  static std::unordered_set<std::string> Set(FilterPasses.begin(),
+                                             FilterPasses.end());
+  return Set.empty() || Set.count(std::string(PassName));
+}
+
 bool llvm::isFunctionInPrintList(StringRef FunctionName) {
   static std::unordered_set<std::string> PrintFuncNames(PrintFuncsList.begin(),
                                                         PrintFuncsList.end());
   return PrintFuncNames.empty() ||
          PrintFuncNames.count(std::string(FunctionName));
 }
 
 std::string llvm::doSystemDiff(StringRef Before, StringRef After,

llvm/lib/Passes/StandardInstrumentations.cpp

 #else
     cl::init(true)
 #endif
     );
 
 // An option that supports the -print-changed option.  See
 // the description for -print-changed for an explanation of the use
 // of this option.  Note that this option has no effect without -print-changed.
-static cl::list<std::string>
-    PrintPassesList("filter-passes", cl::value_desc("pass names"),
-                    cl::desc("Only consider IR changes for passes whose names "
-                             "match for the print-changed option"),
-                    cl::CommaSeparated, cl::Hidden);
-// An option that supports the -print-changed option.  See
-// the description for -print-changed for an explanation of the use
-// of this option.  Note that this option has no effect without -print-changed.
 static cl::opt<bool>
     PrintChangedBefore("print-before-changed",
                        cl::desc("Print before passes that change them"),
                        cl::init(false), cl::Hidden);
 
 // An option for specifying the dot used by
 // print-changed=[dot-cfg | dot-cfg-quiet]
 static cl::opt<std::string>
return any_cast<const LazyCallGraph::SCC *>(IR)
         .getParent();
   return nullptr;
 }
 
 bool isInterestingFunction(const Function &F) {
   return isFunctionInPrintList(F.getName());
 }
 
-bool isInterestingPass(StringRef PassID) {
-  if (isIgnored(PassID))
-    return false;
-
-  static std::unordered_set<std::string> PrintPassNames(PrintPassesList.begin(),
-                                                        PrintPassesList.end());
-  return PrintPassNames.empty() || PrintPassNames.count(PassID.str());
-}
-
 // Return true when this is a pass on IR for which printing
 // of changes is desired.
 bool isInteresting(Any IR, StringRef PassID) {
-  if (!isInterestingPass(PassID))
+  if (isIgnored(PassID) || !isPassInPrintList(PassID))
     return false;
   if (any_isa<const Function *>(IR))
     return isInterestingFunction(*any_cast<const Function *>(IR));
   return true;
 }
 
 } // namespace
 

llvm/test/Other/ChangePrinters/print-changed-diff-machine.ll

 ; REQUIRES: aarch64-registered-target
 ; RUN: llc -filetype=null -mtriple=aarch64 -O0 -print-changed=diff %s 2>&1 | FileCheck %s --check-prefixes=DIFF,VERBOSE
 ; RUN: llc -filetype=null -mtriple=aarch64 -O0 -print-changed=diff-quiet %s 2>&1 | FileCheck %s --check-prefixes=DIFF,QUIET
 ; RUN: llc -filetype=null -mtriple=aarch64 -O0 -print-changed=cdiff %s 2>&1 | FileCheck %s --check-prefixes=CDIFF,VERBOSE
 ; RUN: llc -filetype=null -mtriple=aarch64 -O0 -print-changed=cdiff-quiet %s 2>&1 | FileCheck %s --check-prefixes=CDIFF,QUIET
 
-; VERBOSE:    *** IR Dump After AArch64O0PreLegalizerCombiner on foo omitted because no change ***
+; VERBOSE:    *** IR Dump After AArch64O0PreLegalizerCombiner (aarch64-O0-prelegalizer-combiner) on foo omitted because no change ***
 ; QUIET-NOT:  *** {{.*}} omitted because no change ***
 
 ; DIFF:      *** IR Dump After Legalizer (legalizer) on foo ***
 ; DIFF-NEXT: -# Machine code for function foo: IsSSA, TracksLiveness
 ; DIFF-NEXT: +# Machine code for function foo: IsSSA, TracksLiveness, Legalized
 ; DIFF-NEXT:  Function Live Ins: $w0
 
 ; CDIFF:      *** IR Dump After Legalizer (legalizer) on foo ***

llvm/test/Other/print-changed-machine.ll

 ; REQUIRES: aarch64-registered-target
 ; RUN: llc -filetype=null -mtriple=aarch64 -O0 -print-changed %s 2>&1 | FileCheck %s --check-prefixes=VERBOSE,VERBOSE-BAR
 ; RUN: llc -filetype=null -mtriple=aarch64 -O0 -print-changed -filter-print-funcs=foo %s 2>&1 | FileCheck %s --check-prefixes=VERBOSE,NO-BAR
 
 ; VERBOSE:       *** IR Dump After IRTranslator (irtranslator) on foo ***
 ; VERBOSE-NEXT:  # Machine code for function foo: IsSSA, TracksLiveness{{$}}
 ; VERBOSE-NEXT:  Function Live Ins: $w0
 ; VERBOSE-EMPTY:
 ; VERBOSE-NEXT:  bb.1.entry:
-; VERBOSE:       *** IR Dump After Analysis for ComputingKnownBits on foo omitted because no change ***
-; VERBOSE-NEXT:  *** IR Dump After AArch64O0PreLegalizerCombiner on foo omitted because no change ***
+; VERBOSE:       *** IR Dump After Analysis for ComputingKnownBits (gisel-known-bits) on foo omitted because no change ***
+; VERBOSE-NEXT:  *** IR Dump After AArch64O0PreLegalizerCombiner (aarch64-O0-prelegalizer-combiner) on foo omitted because no change ***
 ; VERBOSE:       *** IR Dump After Legalizer (legalizer) on foo ***
 ; VERBOSE-NEXT:  # Machine code for function foo: IsSSA, TracksLiveness, Legalized
 ; VERBOSE-NEXT:  Function Live Ins: $w0
 ; VERBOSE-EMPTY:
 ; VERBOSE-NEXT:  bb.1.entry:
 
 ; VERBOSE-BAR:   *** IR Dump After IRTranslator (irtranslator) on bar ***
 ; NO-BAR-NOT:    on bar ***
 
 ; RUN: llc -filetype=null -mtriple=aarch64 -O0 -print-changed=quiet %s 2>&1 | FileCheck %s --check-prefix=QUIET
 
 ; QUIET:         *** IR Dump After IRTranslator (irtranslator) on foo ***
 ; QUIET-NOT:     ***
 ; QUIET:         *** IR Dump After Legalizer (legalizer) on foo ***
 
+; RUN: llc -filetype=null -mtriple=aarch64 -O0 -print-changed -filter-passes=irtranslator,legalizer %s 2>&1 | \
+; RUN:   FileCheck %s --check-prefixes=VERBOSE-FILTER
+; RUN: llc -filetype=null -mtriple=aarch64 -O0 -print-changed=quiet -filter-passes=irtranslator %s 2>&1 | \
+; RUN:   FileCheck %s --check-prefixes=QUIET-FILTER --implicit-check-not='IR Dump'
+
+; VERBOSE-FILTER:      *** IR Dump After IRTranslator (irtranslator) on foo ***
+; VERBOSE-FILTER:      *** IR Dump After AArch64O0PreLegalizerCombiner (aarch64-O0-prelegalizer-combiner) on foo filtered out ***
+; VERBOSE-FILTER:      *** IR Dump After Legalizer (legalizer) on foo ***
+; VERBOSE-FILTER-NOT:  *** IR Dump After {{.*}} () on
+
+; QUIET-FILTER: *** IR Dump After IRTranslator (irtranslator) on foo ***
+; QUIET-FILTER: *** IR Dump After IRTranslator (irtranslator) on bar ***
+
 ;; dot-cfg/dot-cfg-quiet are unimplemented. Currently they behave like 'quiet'.
 ; RUN: llc -filetype=null -mtriple=aarch64 -O0 -print-changed=dot-cfg %s 2>&1 | FileCheck %s --check-prefix=QUIET
 
 @var = global i32 0
 
 define void @foo(i32 %a) {
 entry:
   %b = add i32 %a, 1