- 
                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.