Skip to content

[GlobalISel] Combiner: Install Observer into MachineFunction #102156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

tobias-stadler
Copy link
Contributor

The Combiner doesn't install the Observer into the MachineFunction.
This probably went unnoticed, because MachineFunction::getObserver() is
currently only used in constrainOperandRegClass(), but this might cause
issues down the line.

@llvmbot
Copy link
Member

llvmbot commented Aug 6, 2024

@llvm/pr-subscribers-llvm-globalisel

Author: Tobias Stadler (tobias-stadler)

Changes

The Combiner doesn't install the Observer into the MachineFunction.
This probably went unnoticed, because MachineFunction::getObserver() is
currently only used in constrainOperandRegClass(), but this might cause
issues down the line.


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

1 Files Affected:

  • (modified) llvm/lib/CodeGen/GlobalISel/Combiner.cpp (+1-1)
diff --git a/llvm/lib/CodeGen/GlobalISel/Combiner.cpp b/llvm/lib/CodeGen/GlobalISel/Combiner.cpp
index 3310ce5455c97..5da9e86b20761 100644
--- a/llvm/lib/CodeGen/GlobalISel/Combiner.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/Combiner.cpp
@@ -153,7 +153,7 @@ bool Combiner::combineMachineInstrs() {
     // down RPOT.
     Changed = false;
 
-    RAIIDelegateInstaller DelInstall(MF, ObserverWrapper.get());
+    RAIIMFObsDelInstaller DelInstall(MF, *ObserverWrapper);
     for (MachineBasicBlock *MBB : post_order(&MF)) {
       for (MachineInstr &CurMI :
            llvm::make_early_inc_range(llvm::reverse(*MBB))) {

Created using spr 1.3.6-bogner-wip
Copy link
Contributor

@aemerson aemerson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're also using this in the IRTranslator. Do those need changing at all?

@tobias-stadler
Copy link
Contributor Author

We're also using this in the IRTranslator. Do those need changing at all?

Good catch. I have put up #102379. Those were the last improper uses of RAIIDelegateInstaller.

@tobias-stadler tobias-stadler merged commit bf3aa88 into main Aug 11, 2024
9 checks passed
@tobias-stadler tobias-stadler deleted the users/tobias-stadler/spr/globalisel-combiner-install-observer-into-machinefunction branch August 11, 2024 16:09
tobias-stadler added a commit that referenced this pull request Aug 13, 2024
Similar to #102156.

runOnMachineFunction() installs the Observer correctly manually.

finishPendingPhis() doesn't install into the MF, but this currently
can't cause issues because DILocationVerifier doesn't care about changed
instructions.

Switch to RAIIMFObsDelInstaller in both places for consistency.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants