[llvm-commits] [llvm] r131581 - in /llvm/trunk: include/llvm/DefaultPasses.h include/llvm/Support/StandardPasses.h lib/Support/StandardPasses.cpp

[Chris Lattner]

On May 21, 2011, at 1:15 PM, David Chisnall wrote:
>> The first is clearly useful though, something like:
>> 
>> Builder.addEarlyCGSCCPass(2, new mypass());
>> 
>> Seems reasonable.
> 
> Where would this go, in some static constructor in the pass?  Where does Builder come from?  How does the front end get it, and how does the plugin modify it without adding some tight coupling between front ends and optimisation plugins (which is what I was trying to avoid).

I implemented this, and converted clang to use it.  A hypothetical clang plugin would need access to the PMBuilder while it's under construction, or the plugin would have to arrange for its stuff to get added to the PMBuilder when clang gets around to making a pass list.

>> I understand your position here, but my problem is that this patch should never have gone in in the first place.  One bad part of post-commit review is that sometimes stuff like this is caught late.  Are you ok with reverting it and doing the right thing?  
> 
> This actually was reviewed before committing, by Nick and others, and the final version incorporated their feedback.

Fair enough, this is a problem of code review then, not of post-commit review :)

>> 1. I'll implement the new builder class into a new file, which will have the same functionality as before.
>> 2. Revert your patch.
>> 3. You can add the functionality you need to the new builder.
>> 
>> This gets us to the right position, without requiring your to redesign the whole thing.  Seem reasonable?
> 
> It sounds fine, except that I'm still not completely convinced that your approach actually gives us the functionality.  

Lets keep your patch in until we have a chance to resolve this.  I assume that you're doing your work in the context of dragonegg?  If so, can you switch dragonegg over to the new PassManagerBuilder?

I think that the best way to handle the plugin extensibility problem is to pick the interesting extension points (places where plugins can add passes), and add something like this:

enum PMExtensionPointTy {
  EP_LatePerFunctionPasses,
  EP_EarlyPerModulePasses,
  ..
};

Then plugins can add an API like this:

PMBuilder.addPassExtension(EP_LatePerFunctionPasses, addMyLatePerFunctionExtensions);

Where addMyLatePerFunctionExtensions is declared as something like this:

void addMyLatePerFunctionExtensions(const PMBuilder &Builder, PassManagerBase &PM) {
  if (Builder.getOptLevel() > 2 && Builder.getOptSizeLevel() == 0)
    PM.add(createMyPass());
}

Would something like this work for you?  What are the places that you'd like to add passes, specifically?

-Chris