-
Notifications
You must be signed in to change notification settings - Fork 15k
[MLIR][Python] Call notifyOperationInserted while constructing new op in rewrite patterns
#163694
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
Open
PragmaTwice
wants to merge
2
commits into
llvm:main
Choose a base branch
from
PragmaTwice:mlir-python-listener
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think both the addition of
PyRewriterBaseListenerand the idea of somehow calling notify on listeners is a great idea. i think using it like this (in addition to adding it to the default threadcontextstack) is not a good idea. 2 possibilities:add(here) explicitlyalso btw ADT isn't actually safe to use like this i think? i need to double checklooks like IRAttributes already uses ScopeExit 🤷Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup it's possible and the code can be simpler by doing this. and even we don't need to make
listenera python object.we can just have a threadlocal current listener instead of a stack of listeners, but to allow users to do weird nested things like this:
.. a stack can be more general (but i'm also not sure since this seems an anti-pattern..)
this can be a little hard since
listeneris like a field ofPatternRewriter, and outside the callback ofaddwe cannot access therewritereasily.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the "core" way to do this is ForwardingListener ie the listener "chains" have to actually be constructed at instantiation of the leaf listener (which then forwards up the chain or something like that). but that's too complicated for right now before anyone actually needs it (asks for it). let's leave off this stacks/chains of listeners for now.
can't you play the same trick we play everywhere already: just put it into
*userData? basically add anaddoverload that takes alistenerand sticks it into*userDataalong with the callable?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basically i guess i'm saying the listener doesn't have to be on the
PatternRewriterjust because it's like that in cpp...Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll refactor this change soon.
yeah I can get your point, but we cannot get the listener before we get the rewriter, and only after we enter the callback we can finally get the rewriter. (shown as an example below)
the trick here is that, we need to get the listener inside every
Operation.create(..)and wrappers likearith.addi(..), so we should maintain a global state for retrieving the current listener. (shown below)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gimme a couple minutes to try your patch...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here's a sketch of what i'm talking about (on top of your current commit):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your patch! I understand the code, but since we can already get the listener via
nb::object listener = nb::cast(pyRewriter.getListener())without passing additional user data, is there any difference?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahhh I think I've got some clue about the inconsistency between our understanding.
I want to explain in a gradual way why we need a global state (or stack). Firstly, we can just throw these stack/state things if we write code like this:
Here we pass the listener (or the rewriter) into the op constructor so we don't need any global state, and then we can use this listener to call
notify..methods. But in the initial I want to avoid such passing, so some code like this:And then, obviously, we need some global state to know which is the current listener that we need to obtain in the op constructor to call notify methods.
If we want to avoid global state/stack maybe we can just pass the listener to op constructors? this may require some changes to tblgen.