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

[David Chisnall]

On 21 May 2011, at 20:18, Chris Lattner wrote:

> On May 21, 2011, at 11:38 AM, David Chisnall wrote:
> 
>> - Register a new pass to run at optimisation level 2
>> - Register a new pass to run at any optimisation level, except when we are optimising for size
>> - Replace an existing pass with a different implementation
>> - Register a new pass to run at optimisation level 2, before another pass from another plugin, and only if that other plugin is actually loaded
> 
> It is unclear to me that we need or want this level of control.  

Well, the passes that I have, some increase the size considerably, so I don't want them to run when optimising for size.  

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

> 
>>> Are you ok with reverting the patch and trying a different approach?
>> 
>> I'd rather not revert it, since I'm using this functionality, but I've no problem with replacing it with something better - as far as I'm concerned the design is not finalised until 3.0 ships, and I'm happy to make changes to it, including significant architectural changes, as long as we end up with something useable at the end.  
> 
> 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.

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

David