[llvm-commits] [llvm] r131155 - in /llvm/trunk: include/llvm/PassSupport.h include/llvm/Support/StandardPasses.h lib/VMCore/StandardPasses.cpp

[Frits van Bommel]

Oops, forgot to CC the list.

From: Frits van Bommel <fvbommel at gmail.com>
Date: Wed, May 11, 2011 at 12:32 AM
Subject: Re: [llvm-commits] [llvm] r131155 - in /llvm/trunk:
include/llvm/PassSupport.h include/llvm/Support/StandardPasses.h
lib/VMCore/StandardPasses.cpp
To: David Chisnall <csdavec at swan.ac.uk>

On Tue, May 10, 2011 at 11:36 PM, David Chisnall <csdavec at swan.ac.uk> wrote:
> +  /// Returns the flags that must not be set for this to match
> +  static unsigned DisallowedFlags(unsigned flags) {
> +      return (flags & DisallowedFlagMask) >> RequiredFlagShift;

I think you meant 'DisallowedFlagShift' here.

> +  }

> +void StandardPass::RegisterDefaultPass(PassInfo::NormalCtor_t constructor,
> +                                       unsigned char *newPass,
> +                                       unsigned char *oldPass,
> +                                       StandardPass::StandardSet set,
> +                                       unsigned flags) {
> +  // Make sure that the standard sets are already regstered
> +  RegisterDefaultStandardPasses(RegisterDefaultPasses);
> +  // Get the correct list to modify
> +  llvm::SmallVectorImpl<StandardPassEntry> &passList = PassList(set);
> +
> +  // If there is no old pass specified, then we are adding a new final pass, so
> +  // just push it onto the end.
> +  if (!oldPass) {
> +    StandardPassEntry pass(constructor, newPass, flags);
> +    passList.push_back(pass);
> +    return;
> +  }
> +
> +  // Find the correct place to insert the pass.  This is a linear search, but
> +  // this shouldn't be too slow since the SmallVector will store the values in
> +  // a contiguous block of memory.  Each entry is just three words of memory, so
> +  // in most cases we are only going to be looking in one or two cache lines.
> +  // The extra memory accesses from a more complex search structure would
> +  // offset any performance gain (unless someone decides to

Unfinished comment?

> +  for (SmallVectorImpl<StandardPassEntry>::iterator i = passList.begin(),
> +       e=passList.end(); i != e; ++i) {
> +    if (i->passID == oldPass) {
> +      StandardPassEntry pass(constructor, newPass, flags);
> +      passList.insert(i, pass);
> +      // If we've added a new pass, then there may have gained the ability to
> +      // insert one of the previously unresolved ones.  If so, insert the new
> +      // one.
> +      for (SmallVectorImpl<UnresolvedStandardPass>::iterator
> +           u = UnresolvedPasses.begin(), eu = UnresolvedPasses.end();
> +           u!=eu; ++u){
> +        if (u->next == newPass && u->set == set) {
> +          UnresolvedStandardPass p = *u;
> +          UnresolvedPasses.erase(u);
> +          RegisterDefaultPass(p.createPass, p.passID, p.next, p.set, p.flags);
> +        }
> +      }
> +      return;

What if 'oldPass' refers to one of those passes that tends to be used
multiple times in a pipeline such as -simplifycfg or -instcombine?
Is the new pass then only inserted once, before the first instance of
the old one? What if a new pass wants to run just before the *second*
time that old pass runs? (for example, because it wants to run after
the pass just before that)

> +    }
> +  }
> +  // If we get to here, then we didn't find the correct place to insert the new
> +  // pass
> +  UnresolvedStandardPass pass(constructor, newPass, oldPass, set, flags);
> +  UnresolvedPasses.push_back(pass);
> +}


It would be nice to have this interface documented somewhere other
than in this code, preferably with an example of how to use it. Maybe
a new section in docs/WritingAnLLVMPass.html?