[CodeGen] Support start/stop in CodeGenPassBuilder #70912

paperchalice · 2023-11-01T08:14:16Z

Add -start/stop-before/after support for CodeGenPassBuilder.
Part of #69879.

llvmbot · 2023-11-01T08:15:30Z

@llvm/pr-subscribers-llvm-ir

Author: None (paperchalice)

Changes

Add a special hook to support -start-*=<passes> and -stop-*=<passes> in llc. If still use registerShouldRunOptionalPassCallback then llc can't skip some passes.
Part of #69879.
NOTE: This hook has a higher priority than PassName::isRequired().

Full diff: https://github.com/llvm/llvm-project/pull/70912.diff

3 Files Affected:

(modified) llvm/include/llvm/IR/PassInstrumentation.h (+33)
(modified) llvm/lib/CodeGen/TargetPassConfig.cpp (+16-3)
(modified) llvm/lib/Passes/StandardInstrumentations.cpp (+4-3)

diff --git a/llvm/include/llvm/IR/PassInstrumentation.h b/llvm/include/llvm/IR/PassInstrumentation.h
index 519a5e46b4373b7..3f70fcf180af81a 100644
--- a/llvm/include/llvm/IR/PassInstrumentation.h
+++ b/llvm/include/llvm/IR/PassInstrumentation.h
@@ -84,6 +84,23 @@ class PassInstrumentationCallbacks {
   using AfterAnalysisFunc = void(StringRef, Any);
   using AnalysisInvalidatedFunc = void(StringRef, Any);
   using AnalysesClearedFunc = void(StringRef);
+  using StartStopFunc = bool(StringRef, Any);
+
+  struct CodeGenStartStopInfo {
+    StringRef Start;
+    StringRef Stop;
+
+    bool IsStopMachinePass = false;
+
+    llvm::unique_function<StartStopFunc> StartStopCallback;
+
+    bool operator()(StringRef PassID, Any IR) {
+      return StartStopCallback(PassID, IR);
+    }
+    bool isStopMachineFunctionPass() const { return IsStopMachinePass; }
+    bool willCompleteCodeGenPipeline() const { return Stop.empty(); }
+    StringRef getStop() const { return Stop; }
+  };
 
 public:
   PassInstrumentationCallbacks() = default;
@@ -148,6 +165,17 @@ class PassInstrumentationCallbacks {
     AnalysesClearedCallbacks.emplace_back(std::move(C));
   }
 
+  void registerStartStopInfo(CodeGenStartStopInfo &&C) {
+    StartStopInfo = std::move(C);
+  }
+
+  bool isStartStopInfoRegistered() const { return StartStopInfo.has_value(); }
+
+  CodeGenStartStopInfo &getStartStopInfo() {
+    assert(StartStopInfo.has_value() && "StartStopInfo is unregistered!");
+    return *StartStopInfo;
+  }
+
   /// Add a class name to pass name mapping for use by pass instrumentation.
   void addClassToPassName(StringRef ClassName, StringRef PassName);
   /// Get the pass name for a given pass class name.
@@ -183,6 +211,8 @@ class PassInstrumentationCallbacks {
   /// These are run on analyses that have been cleared.
   SmallVector<llvm::unique_function<AnalysesClearedFunc>, 4>
       AnalysesClearedCallbacks;
+  /// For `llc` -start-* -stop-* options.
+  std::optional<CodeGenStartStopInfo> StartStopInfo;
 
   StringMap<std::string> ClassToPassName;
 };
@@ -236,6 +266,9 @@ class PassInstrumentation {
         ShouldRun &= C(Pass.name(), llvm::Any(&IR));
     }
 
+    if (Callbacks->StartStopInfo)
+      ShouldRun &= (*Callbacks->StartStopInfo)(Pass.name(), llvm::Any(&IR));
+
     if (ShouldRun) {
       for (auto &C : Callbacks->BeforeNonSkippedPassCallbacks)
         C(Pass.name(), llvm::Any(&IR));
diff --git a/llvm/lib/CodeGen/TargetPassConfig.cpp b/llvm/lib/CodeGen/TargetPassConfig.cpp
index e6ecbc9b03f7149..bea8590a14080ca 100644
--- a/llvm/lib/CodeGen/TargetPassConfig.cpp
+++ b/llvm/lib/CodeGen/TargetPassConfig.cpp
@@ -508,6 +508,9 @@ static void registerPartialPipelineCallback(PassInstrumentationCallbacks &PIC,
   unsigned StopBeforeInstanceNum = 0;
   unsigned StopAfterInstanceNum = 0;
 
+  bool IsStopBeforeMachinePass = false;
+  bool IsStopAfterMachinePass = false;
+
   std::tie(StartBefore, StartBeforeInstanceNum) =
       getPassNameAndInstanceNum(StartBeforeOpt);
   std::tie(StartAfter, StartAfterInstanceNum) =
@@ -536,7 +539,15 @@ static void registerPartialPipelineCallback(PassInstrumentationCallbacks &PIC,
     report_fatal_error(Twine(StopBeforeOptName) + Twine(" and ") +
                        Twine(StopAfterOptName) + Twine(" specified!"));
 
-  PIC.registerShouldRunOptionalPassCallback(
+  std::vector<StringRef> SpecialPasses = {"PassManager", "PassAdaptor",
+                                          "PrintMIRPass", "PrintModulePass"};
+
+  PassInstrumentationCallbacks::CodeGenStartStopInfo Info;
+  Info.Start = StartBefore.empty() ? StartAfter : StartBefore;
+  Info.Stop = StopBefore.empty() ? StopAfter : StopBefore;
+
+  Info.IsStopMachinePass = IsStopBeforeMachinePass || IsStopAfterMachinePass;
+  Info.StartStopCallback =
       [=, EnableCurrent = StartBefore.empty() && StartAfter.empty(),
        EnableNext = std::optional<bool>(), StartBeforeCount = 0u,
        StartAfterCount = 0u, StopBeforeCount = 0u,
@@ -567,8 +578,10 @@ static void registerPartialPipelineCallback(PassInstrumentationCallbacks &PIC,
           EnableCurrent = true;
         if (StopBeforePass && StopBeforeCount++ == StopBeforeInstanceNum)
           EnableCurrent = false;
-        return EnableCurrent;
-      });
+        return EnableCurrent || isSpecialPass(P, SpecialPasses);
+      };
+
+  PIC.registerStartStopInfo(std::move(Info));
 }
 
 void llvm::registerCodeGenCallback(PassInstrumentationCallbacks &PIC,
diff --git a/llvm/lib/Passes/StandardInstrumentations.cpp b/llvm/lib/Passes/StandardInstrumentations.cpp
index 06cc58c0219632d..c6ec1c34d74418d 100644
--- a/llvm/lib/Passes/StandardInstrumentations.cpp
+++ b/llvm/lib/Passes/StandardInstrumentations.cpp
@@ -1036,9 +1036,10 @@ void PrintPassInstrumentation::registerCallbacks(
     SpecialPasses.emplace_back("PassAdaptor");
   }
 
-  PIC.registerBeforeSkippedPassCallback([this, SpecialPasses](StringRef PassID,
-                                                              Any IR) {
-    assert(!isSpecialPass(PassID, SpecialPasses) &&
+  PIC.registerBeforeSkippedPassCallback([this, SpecialPasses,
+                                         &PIC](StringRef PassID, Any IR) {
+    assert((!isSpecialPass(PassID, SpecialPasses) ||
+            PIC.isStartStopInfoRegistered()) &&
            "Unexpectedly skipping special pass");
 
     print() << "Skipping pass: " << PassID << " on " << getIRName(IR) << "\n";

llvm/include/llvm/IR/PassInstrumentation.h

aeubanks · 2024-01-02T20:43:31Z

instead of doing this in the instrumentation, can we do this by not adding the pass to begin with? that seems cleaner.

e.g. AddMachinePass can check if we've added some pass and bail if we have/haven't seen the pass so far

paperchalice · 2024-01-03T04:09:42Z

instead of doing this in the instrumentation, can we do this by not adding the pass to begin with? that seems cleaner.

e.g. AddMachinePass can check if we've added some pass and bail if we have/haven't seen the pass so far

Update for this now. Add a callback to PassInstrumentationCallbacks now, Add*Pass can decide whether to add pass through this callback.

The work of determining whether a pass is a machine pass will be completed by the pass builder, which requires #76320.

Update: In this way, it may not recognize some passes which is added by adaptor class, like LoopStrengthReducePass.

aeubanks · 2024-01-05T07:13:52Z

sorry, been swamped with unbreaking things this week, will try to review next week

paperchalice · 2024-01-07T02:10:29Z

If use this way, I'd like to finish #76320 and #77084 firstly so we can at least verify the pass pipeline string in unittest.

aeubanks

yes, I think this makes more sense, thanks

aeubanks · 2024-01-09T22:29:55Z

llvm/include/llvm/CodeGen/CodeGenPassBuilder.h

-        LoopStrengthReducePass(), /*UseMemorySSA*/ true, Opt.DebugPM));
+    addPass(createFunctionToLoopPassAdaptor(LoopStrengthReducePass(),
+                                            /*UseMemorySSA*/ true, Opt.DebugPM),
+            LoopStrengthReducePass::name());


this is unfortunate, I wonder if we should just make this a function pass, but we can do that in the future

aeubanks · 2024-01-10T05:40:05Z

llvm/lib/IR/PassInstrumentation.cpp

@@ -28,6 +28,14 @@ PassInstrumentationCallbacks::getPassNameForClassName(StringRef ClassName) {
  return ClassToPassName[ClassName];
 }

+StringRef
+PassInstrumentationCallbacks::getClassNameForPassName(StringRef PassName) {


rather than adding this reverse map, let's be consistent with existing instrumentation like here where we check the pass name against PIC->getPassNameForClassName(PassID) in the callbacks. even if this is slower it doesn't really matter since this is just used for testing iiuc

Then we may need #76320 to populate pass names.

aeubanks · 2024-01-10T05:42:51Z

llvm/include/llvm/IR/PassInstrumentation.h

+  StringRef getClassNameForPassName(StringRef PassName);
+
+  /// Helper callback to support options like start-before.
+  llvm::unique_function<StartStopFunc> StartStopCallback;


I'm missing where this is being used.

Also, PassInstrumentationCallbacks doesn't really seem like the right place to put this. The only reason we interact with PIC is to get the class/pass name map. If we can somehow separate that out, that would be ideal, but perhaps that's too hard.

Oh, I forgot to remove this, this implementation doesn't need this any more, sorry.

paperchalice · 2024-01-14T13:09:42Z

Add simple unit test and will move to lit test in future.
Is -stop-before=free-machine-function/-stop-after=asm-printer a meaningful option?

aeubanks

lg with some comments

we really should get basic llc support soon

aeubanks · 2024-01-16T21:37:36Z

llvm/include/llvm/CodeGen/CodeGenPassBuilder.h

      static_assert((is_detected<is_function_pass_t, PassT>::value ||
                     is_detected<is_module_pass_t, PassT>::value) &&
                    "Only module pass and function pass are supported.");

      // Add Function Pass
      if constexpr (is_detected<is_function_pass_t, PassT>::value) {
+        if (!PB.runBeforeAdding(Name))
+          return;


can factor this out of if/else branches

aeubanks · 2024-01-16T22:03:47Z

llvm/include/llvm/CodeGen/CodeGenPassBuilder.h

-        C(&PassT::Key);
+
+      for (auto &C : PB.AfterCallbacks)
+        C(PassT::name());
    }

    template <typename PassT> void insertPass(MachinePassKey *ID, PassT Pass) {


unrelated, and I think I mentioned this before, but I'd like to see if we can come up with a nicer way of doing this, perhaps with something like PassBuilder extension points (e.g. PassBuilder::registerPeepholeEPCallback()). it seems in line with various per-target TargetMachines adding their own passes anyway. do you know if we could potentially replace insertPass() with the various TargetMachines simply adding passes in the right place, or are the passes they add too ad hoc?

Currently AMDGPU, Hexagon, PowerPC and RISCV uses insertPass to adjust the regalloc pipeline, we need add extension points after PHIElimination, TwoAddressInstruction, MachineScheduler, RenameIndependentSubregs, LiveVariables and DetectDeadLanes. They inject passes between these passes directly and only happens in regalloc pipeline.

aeubanks · 2024-01-16T22:05:13Z

llvm/include/llvm/CodeGen/CodeGenPassBuilder.h

@@ -123,6 +124,13 @@ namespace llvm {
 /// all of the built-in passes, and those may reference these members during
 /// construction.
 template <typename DerivedT> class CodeGenPassBuilder {
+  bool runBeforeAdding(StringRef Name) const {


can you move this definition somewhere else in the class? it doesn't belong right at the top

aeubanks · 2024-01-16T22:06:07Z

llvm/include/llvm/CodeGen/CodeGenPassBuilder.h

+
+  Error verifyStartStop(const TargetPassConfig::StartStopInfo &Info) const;
+
+  mutable SmallVector<llvm::unique_function<bool(StringRef)>, 4>


it seems nicer to keep this in AddPass. is the reason that these are moved out of AddPass because it references Started/Stopped in CodeGenPassBuilder? and there's no way to easily validate them if these are somewhere else?

Currently IR passes and machine function passes are handled by different classes, and -start/stop options should support both IR and machine function passes, but it's OK to merge them into one class, because only buildPipeline creates instance of AddIRPass and AddMachinePass, but IMHO keeping these in CodeGenPassBuilder can let target disable passes in constructor.

paperchalice · 2024-01-18T04:06:33Z

Address some comments.

Reverts #70912. This breaks some bazel tests.

Add `-start/stop-before/after` support for CodeGenPassBuilder. Part of llvm#69879.

…78570) Unfortunately the legacy pass system can't recognize `no-op-module` and `no-op-function` so it causes test failure in `CodeGenTests`. Add a workaround in function `PassInfo *getPassInfo(StringRef PassName)`, `TargetPassConfig.cpp`.

llvmbot added the llvm:ir label Nov 1, 2023

paperchalice mentioned this pull request Nov 2, 2023

[NewPM][CodeGen][llc] Add NPM support #70922

Merged

paperchalice requested a review from aeubanks December 22, 2023 00:23

arsenm approved these changes Dec 22, 2023

View reviewed changes

llvm/include/llvm/IR/PassInstrumentation.h Outdated Show resolved Hide resolved

paperchalice force-pushed the startstop branch from d7b90d6 to 9a4d24c Compare December 22, 2023 08:40

paperchalice force-pushed the startstop branch 3 times, most recently from 1cbce28 to 5e9c1dc Compare January 3, 2024 03:54

paperchalice force-pushed the startstop branch from 5e9c1dc to b14ba16 Compare January 7, 2024 02:08

paperchalice force-pushed the startstop branch 3 times, most recently from 150903d to ab89ac7 Compare January 8, 2024 02:37

paperchalice marked this pull request as draft January 8, 2024 05:57

paperchalice force-pushed the startstop branch from ab89ac7 to 351462a Compare January 8, 2024 05:58

paperchalice mentioned this pull request Jan 9, 2024

[NewPM][CodeGen] add TargetPassConfig like API #70906

Open

paperchalice force-pushed the startstop branch from 351462a to 16d2498 Compare January 9, 2024 06:45

aeubanks reviewed Jan 10, 2024

View reviewed changes

paperchalice force-pushed the startstop branch 3 times, most recently from d7fa9a8 to 6eb14fe Compare January 10, 2024 06:08

pravinjagtap self-requested a review January 11, 2024 12:51

paperchalice force-pushed the startstop branch from 6eb14fe to 5ef19fa Compare January 13, 2024 10:31

paperchalice changed the title ~~[Pass] Support start/stop in instrumentation~~ [CodeGen] Support start/stop in CodeGenPassBuilder Jan 13, 2024

paperchalice marked this pull request as ready for review January 13, 2024 10:32

paperchalice force-pushed the startstop branch from 5ef19fa to 509b1cf Compare January 14, 2024 04:20

paperchalice requested review from arsenm and aeubanks January 14, 2024 10:46

aeubanks approved these changes Jan 16, 2024

View reviewed changes

[CodeGen][NewPM] Support start/stop in CodeGen

ce88f2b

paperchalice force-pushed the startstop branch from 509b1cf to ce88f2b Compare January 17, 2024 01:04

paperchalice merged commit baaf0c9 into llvm:main Jan 18, 2024
4 checks passed

paperchalice mentioned this pull request Jan 18, 2024

Revert "[CodeGen] Support start/stop in CodeGenPassBuilder" #78567

Merged

paperchalice added a commit that referenced this pull request Jan 18, 2024

Revert "[CodeGen] Support start/stop in CodeGenPassBuilder" (#78567)

a48c1bd

Reverts #70912. This breaks some bazel tests.

paperchalice added a commit to paperchalice/llvm-project that referenced this pull request Jan 19, 2024

[CodeGen] Support start/stop in CodeGenPassBuilder (llvm#70912)

bd13755

Add `-start/stop-before/after` support for CodeGenPassBuilder. Part of llvm#69879.

ampandey-1995 pushed a commit to ampandey-1995/llvm-project that referenced this pull request Jan 19, 2024

[CodeGen] Support start/stop in CodeGenPassBuilder (llvm#70912)

7cde12b

Add `-start/stop-before/after` support for CodeGenPassBuilder. Part of llvm#69879.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CodeGen] Support start/stop in CodeGenPassBuilder #70912

[CodeGen] Support start/stop in CodeGenPassBuilder #70912

paperchalice commented Nov 1, 2023 •

edited

llvmbot commented Nov 1, 2023

aeubanks commented Jan 2, 2024

paperchalice commented Jan 3, 2024 •

edited

aeubanks commented Jan 5, 2024

paperchalice commented Jan 7, 2024

aeubanks left a comment

aeubanks Jan 9, 2024

aeubanks Jan 10, 2024

paperchalice Jan 10, 2024

aeubanks Jan 10, 2024

paperchalice Jan 10, 2024

paperchalice commented Jan 14, 2024 •

edited

aeubanks left a comment

aeubanks Jan 16, 2024

aeubanks Jan 16, 2024

paperchalice Jan 17, 2024 •

edited

aeubanks Jan 16, 2024

aeubanks Jan 16, 2024

paperchalice Jan 17, 2024 •

edited

paperchalice commented Jan 18, 2024


		Error verifyStartStop(const TargetPassConfig::StartStopInfo &Info) const;

		mutable SmallVector<llvm::unique_function<bool(StringRef)>, 4>

[CodeGen] Support start/stop in CodeGenPassBuilder #70912

[CodeGen] Support start/stop in CodeGenPassBuilder #70912

Conversation

paperchalice commented Nov 1, 2023 • edited

llvmbot commented Nov 1, 2023

aeubanks commented Jan 2, 2024

paperchalice commented Jan 3, 2024 • edited

aeubanks commented Jan 5, 2024

paperchalice commented Jan 7, 2024

aeubanks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paperchalice commented Jan 14, 2024 • edited

aeubanks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paperchalice Jan 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paperchalice Jan 17, 2024 • edited

Choose a reason for hiding this comment

paperchalice commented Jan 18, 2024

paperchalice commented Nov 1, 2023 •

edited

paperchalice commented Jan 3, 2024 •

edited

paperchalice commented Jan 14, 2024 •

edited

paperchalice Jan 17, 2024 •

edited

paperchalice Jan 17, 2024 •

edited