[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 11:38 AM, David Chisnall wrote:
>> I don't like the patch for several reasons:
>> 1. Tons of macros.
> 
> There aren't many.  As I recall, there are none that are actually exposed, there are just 4 for avoiding copy-and-paste coding in StandardPasses.h

There are many uses of the macros:
+      DEFAULT_MODULE_PASS(InstructionCombining, 0);
+      // Canonicalize indvars
+      DEFAULT_MODULE_PASS(IndVarSimplify, 0);
+      // Recognize idioms like memset.
+      DEFAULT_MODULE_PASS(LoopIdiom, 0);
+      // Delete dead loops
...

>> 2. Exposing tons of PassID's.
> 
> True.  In fact, this doesn't expose as many as are really required, because passes that appear multiple times in the sequence should have unique IDs, to allow just one to be replaced.  
> 
>> 3. You added a bunch of static constructors for the static globals in the new .cpp file.
> 
> I'm not sure where you're talking about here.  Can you be more specific?

These:
+/// Standard alias analysis passes
+static llvm::SmallVector<StandardPassEntry, 4> AAPasses;
+/// Standard function passes
+static llvm::SmallVector<StandardPassEntry, 32> FunctionPasses;
+/// Standard module passes
+static llvm::SmallVector<StandardPassEntry, 32> ModulePasses;
+/// Standard link-time optimization passes
+static llvm::SmallVector<StandardPassEntry, 32> LTOPasses;

force code to be executed at startup time or any application that links in this code, regardless of whether a pass manager is actually dynamically created.  This hurts clients of the clang parser like the static analyzer, for example.

>> Of these, the third is the easiest to solve.  However, can we take a step back and see what problem you'd trying to solve?
>> 
>> While it's theoretically possible to want to control every possible aspect of the standard pipeline from a plugin, I don't think that it is realistically needed.  Instead, I think there are a few different classes of optimization pass, for example:
>> 
>> 1. Something that wants to run at the very start, or very end of the pipeline. (can be any pass)
>> 2. Something that runs after the early per-function passes. (must be a functionpass)
>> 3. Something that runs early in the CGSCC passmgr.  (must be a cgsccpass or functionpass)
>> 4. Something that runs late in the functionpass pipeline (must be a functionpass)
>> 
>> I don't know where you pass fits in, but I'd guess it is #4.
> 
> Some of mine are ModulePasses (e.g. caching the class lookup - a function pass wouldn't be able to share the caches globally).  
> 
> The current code doesn't yet allow replacing existing passes, but it's something that I'd like to add.  For example, I can easily imagine that someone would write a better alias analysis pass, or a pass that had some functionality that overlapped with one of the default set and wanted to replace it.

Sure, we can always add more categories if we need them.

>> Instead of your current approach, how about we introduce a new "OptimizerBuilder" class. The API would be something like:
>> 
>> OptimizerBuilder Builder;
>> Builder.setOptimizationLevel(2);
>> Builder.addEarlyCGSCCPass(new mypass());
>> Builder.addLastFunctionPass(new myotherpass());
>> Builder.populatePassManager(PM);
>> 
>> I think that something like this would give us the extensibility that you need, and would clean up the previously existing nasty API for setting up the pass manager.  What do you think?  
> 
> I'm not sure how this would be used.  Can you give some concrete examples of how, with this scheme, a plugin would:
> 
> - 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.  The first is clearly useful though, something like:

Builder.addEarlyCGSCCPass(2, new mypass());

Seems reasonable.

>> 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?  I want to make it really clear that I agree with your goals, just not this implementation approach.

-Chris