[Instrumentation] Support MachineFunction in ChangeReporter #80946

paperchalice · 2024-02-07T07:36:01Z

No description provided.

llvm/test/Other/ChangePrinters/DotCfg/print-changed-dot-cfg.mir

paperchalice · 2024-02-07T13:39:48Z

llvm/lib/Passes/StandardInstrumentations.cpp

+  Out << "*** MIR Dump At Start ***\n";
+  for (const Function &F : M)
+    if (auto *MF = MMI.getMachineFunction(F))
+      MF->print(Out);


In fact, not all machine functions are available at this time, when running default codegen pipeline.

that's a good point. I'm not sure if we should print something to tell the user that the machine function doesn't exist.

it is weird to print MIR Dump At Start and then not print anything if we don't have any MFs yet. perhaps we just don't print anything here if we don't have any MFs

aeubanks · 2024-02-07T21:30:05Z

llvm/lib/Passes/StandardInstrumentations.cpp

+  Out << "*** MIR Dump At Start ***\n";
+  for (const Function &F : M)
+    if (auto *MF = MMI.getMachineFunction(F))
+      MF->print(Out);


that's a good point. I'm not sure if we should print something to tell the user that the machine function doesn't exist.

it is weird to print MIR Dump At Start and then not print anything if we don't have any MFs yet. perhaps we just don't print anything here if we don't have any MFs

aeubanks · 2024-02-07T21:36:09Z

llvm/lib/CodeGen/MachinePassManager.cpp

@@ -90,7 +90,9 @@ Error MachineFunctionPassManager::run(Module &M,
      for (unsigned I = Begin, E = Idx; I != E; ++I) {
        auto *P = Passes[I].get();

-        if (!PI.runBeforePass<MachineFunction>(*P, MF))
+        // Keep BeforeStack empty in ChangeReporter
+        if (P->name() != FreeMachineFunctionPass::name() &&


we should invoke runAfterPassInvalidated below if we see that we're running FreeMachineFunctionPass, rather than not running instrumentation on it

I have something that does this in my rewrite

we can keep this for now, but #81068 will overwrite this

aeubanks

lg with or without the "no MIR functions to print" comment resolved

aeubanks · 2024-02-08T01:01:11Z

llvm/lib/CodeGen/MachinePassManager.cpp

@@ -90,7 +90,9 @@ Error MachineFunctionPassManager::run(Module &M,
      for (unsigned I = Begin, E = Idx; I != E; ++I) {
        auto *P = Passes[I].get();

-        if (!PI.runBeforePass<MachineFunction>(*P, MF))
+        // Keep BeforeStack empty in ChangeReporter
+        if (P->name() != FreeMachineFunctionPass::name() &&


we can keep this for now, but #81068 will overwrite this

aeubanks · 2024-02-08T22:53:45Z

for simplicity, I think we should just not print the initial MIR since it's unclear exactly what the right thing to do is

aeubanks · 2024-02-09T18:08:24Z

should we just completely drop handleInitialMIR()?

paperchalice · 2024-02-10T03:42:17Z

should we just completely drop handleInitialMIR()?

CFG instruments may benefit from this in verbose mode, but remove it is also ok.

aeubanks

lgtm after comments addressed

aeubanks · 2024-02-12T18:17:15Z

llvm/lib/Passes/StandardInstrumentations.cpp

@@ -448,6 +477,11 @@ template <typename T> void TextChangeReporter<T>::handleInitialIR(Any IR) {
  M->print(Out, nullptr);
 }

+template <typename T>
+void TextChangeReporter<T>::handleInitialMIR(const MachineFunction *IR) {
+  // For simplicity, don't print the initial MIR.


add the reason that it's likely that the MIR doesn't initially exist

aeubanks · 2024-02-12T18:17:43Z

llvm/lib/Passes/StandardInstrumentations.cpp

@@ -362,6 +384,13 @@ void ChangeReporter<T>::saveIRBeforePass(Any IR, StringRef PassID,
      handleInitialIR(IR);


should we be calling this on MIR?

Found previous implementation has some problem, will try to combine handleInitialIR and handleInitialMIR.

aeubanks · 2024-02-12T18:20:17Z

btw we should probably start looking into how to get the hard passes ported, like SelectionDAG or the AsmPrinters

aeubanks · 2024-03-07T21:17:01Z

llvm/lib/Passes/StandardInstrumentations.cpp

-    InitialIR = false;
-    if (VerboseMode)
-      handleInitialIR(IR);
+    if (const auto *MF = unwrapIR<MachineFunction>(IR); !MF || !MF->empty()) {


is the intention here to make the output of instruction selection the "initial IR"?

I feel like being explicit about the fact that instruction selection is actually modifying the MachineFunction is reasonable, so the initial IR being empty isn't too bad. but I'm open to this as well

aeubanks · 2024-03-07T21:19:38Z

llvm/lib/Passes/StandardInstrumentations.cpp

@@ -409,6 +435,10 @@ template <typename T>
 void ChangeReporter<T>::handleInvalidatedPass(StringRef PassID) {
  assert(!BeforeStack.empty() && "Unexpected empty stack encountered.");

+  // Prepare to process the next MIR.
+  if (PassID == FreeMachineFunctionPass::name())


can you explain why this is necessary?

I think in general I'm not understanding the intent with "InitialIR" in regards to MIR. Previously we'd always start with the entire module as the initial IR right? Why are we handling MachineFunctions differently?

It's hard to determine when the initial MIR for a function is created. If the input is general LLVM IR and the pipeline is not interrupted by a module pass then new machine function will not be created until FreeMachineFunction, which is the initial MIR for the next function.

InitialIR is used to determine when to call handleInitialIR, which is typically used to print to the user "Initial IR: ...". I don't think we need that for MIR, except in the case where we start with MIR (mostly just tests). We can just let before/after pass handle MIR?

Let before/after pass handle MIR now, but still print all available MIRs when print-module-scope is on.

paperchalice · 2024-04-09T10:51:14Z

Ping? Now empty machine functions are no longer treated specially.

Fix failure in #80946.

paperchalice requested review from arsenm and aeubanks February 7, 2024 07:36

arsenm reviewed Feb 7, 2024

View reviewed changes

llvm/test/Other/ChangePrinters/DotCfg/print-changed-dot-cfg.mir Outdated Show resolved Hide resolved

paperchalice commented Feb 7, 2024

View reviewed changes

aeubanks reviewed Feb 7, 2024

View reviewed changes

aeubanks approved these changes Feb 8, 2024

View reviewed changes

aeubanks approved these changes Feb 12, 2024

View reviewed changes

paperchalice force-pushed the NPM/CodeGen/si branch 2 times, most recently from a547f69 to 32fcaa3 Compare February 13, 2024 06:48

arsenm approved these changes Feb 29, 2024

View reviewed changes

paperchalice force-pushed the NPM/CodeGen/si branch 2 times, most recently from 28f6547 to a8d5576 Compare March 2, 2024 14:36

aeubanks reviewed Mar 7, 2024

View reviewed changes

paperchalice mentioned this pull request Mar 31, 2024

Enabling new pass manager and Printing changed IRs conflict and crash llc #87061

Closed

paperchalice force-pushed the NPM/CodeGen/si branch 2 times, most recently from 0442446 to b4726bd Compare April 9, 2024 10:49

paperchalice force-pushed the NPM/CodeGen/si branch from b4726bd to 6db1fd7 Compare April 10, 2024 06:34

[Instrumentation] Support MachineFunction in ChangeReporter

73937a2

paperchalice force-pushed the NPM/CodeGen/si branch from 6db1fd7 to 73937a2 Compare April 10, 2024 06:36

aeubanks approved these changes Apr 10, 2024

View reviewed changes

paperchalice merged commit 026165f into llvm:main Apr 11, 2024

paperchalice mentioned this pull request Apr 11, 2024

[NewPM] Fix print-changed-dot-cfg failure #88351

Merged

paperchalice added a commit that referenced this pull request Apr 11, 2024

[NewPM] Fix print-changed-dot-cfg failure (#88351)

efb8cc5

Fix failure in #80946.

paperchalice deleted the NPM/CodeGen/si branch April 12, 2024 04:25

Zentrik pushed a commit to Zentrik/llvm-project that referenced this pull request Jun 29, 2024

[Instrumentation] Support MachineFunction in ChangeReporter (llvm#80946)

63ee8d3

		@@ -362,6 +384,13 @@ void ChangeReporter<T>::saveIRBeforePass(Any IR, StringRef PassID,
		handleInitialIR(IR);

[Instrumentation] Support MachineFunction in ChangeReporter #80946

[Instrumentation] Support MachineFunction in ChangeReporter #80946

Uh oh!

Conversation

paperchalice commented Feb 7, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeubanks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeubanks commented Feb 8, 2024

Uh oh!

aeubanks commented Feb 9, 2024

Uh oh!

paperchalice commented Feb 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aeubanks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aeubanks commented Feb 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paperchalice commented Apr 9, 2024

Uh oh!

Uh oh!

paperchalice commented Feb 10, 2024 •

edited

Loading