- 
                Notifications
    
You must be signed in to change notification settings  - Fork 15.1k
 
[PassBuilder] Add callback invoking to PassBuilder string API #157153
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
| 
          
 ✅ With the latest revision this PR passed the C/C++ code formatter.  | 
    
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.
it'll be nice if you could also add some tests
fa48915    to
    a1320e3      
    Compare
  
    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.
this makes sense, some comments
        
          
                llvm/lib/Passes/PassRegistry.def
              
                Outdated
          
        
      | #undef LOOP_PASS_WITH_PARAMS | ||
| 
               | 
          ||
| #ifdef MODULE_CALLBACK | ||
| MODULE_CALLBACK("PipelineStartCallbacks", invokePipelineStartEPCallbacks) | 
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.
nit: this doesn't follow naming conventions in this file for passes, this should probably be something like callbacks-pipeline-start
        
          
                llvm/lib/Passes/PassBuilder.cpp
              
                Outdated
          
        
      | return true; \ | ||
| } | ||
| #include "PassRegistry.def" | ||
| #undef CGSCC_CALLBACK | 
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.
the other macros are undefed in PassRegistry.def rather than here, why is this different?
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.
Oh, I hadn't seen that. I usually don't #define in one file and #undef in the other but following convention is fine
| 
           I don't have commit bit, so could someone commit for me :)  | 
    
          
 Done, cheers Also I just saw: 
 here is the policy: https://llvm.org/docs/DeveloperPolicy.html#ai-generated-contributions  | 
    
This is a very rough state of what this can look like, but I didn't want to spend too much time on what could be a dead end.
Currently the only way to invoke callbacks is by using the default pipelines, this is an issue if you want to define your own pipeline using the C string API (we do that in LLVM.jl in julia) so I extended the api to allow for invoking those callbacks just like one would call a pass of that kind.
There are some questions about the params that these callbacks take and also I'm missing some of them (some of them are also invoked by the backend so we may not want to expose them)
Code written with AI help, bugs are mine. (Not sure what policy for this is on LLVM)