[llvm] r195241 - [PM] Add the preservation system to the new pass manager.

[Duncan P. N. Exon Smith]

> On 2015-Jan-07, at 03:49, Chandler Carruth <chandlerc at gmail.com> wrote:
> 
> Trying to dig back through stale review that I missed... Sorry for missing them.
> 
> On Fri, May 9, 2014 at 12:15 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> > +    for (SmallPtrSet<void *, 2>::const_iterator I = PreservedPassIDs.begin(),
> > +                                                E = PreservedPassIDs.end();
> > +         I != E; ++I)
> > +      if (!Arg.PreservedPassIDs.count(*I))
> > +        PreservedPassIDs.erase(*I);
> 
> Interesting that SmallPtrSet doesn't let you erase by iterator.  Is
> that an API oversight, or is it intentional?
> 
> I have no idea. I'm happy to take a stab at improving this if you want, but it didn't seem all that bad here. However, I'm not sure I like furthering the "erase doesn't invalidate iterators too much" property of how we do hash tables. =/ That has always seemed like a very fragile guarantee. If we ever move away from it, being able to erase the iterator doesn't help nearly as much (if at all).

That guarantee is pretty fragile, but it doesn't need to be.  A less
fragile (but equally useful) guarantee is, "erase only invalidates
iterators pointing at the erased element".  Then you could either change
this code to:

    // Better guarantee; still requires a bucket lookup for each erase.
    for (auto I = PreservedPassIDs.begin(), E = PreservedPassIDs.end();
         I != E;) {
      auto Next = std::next(I);
      if (!Arg.PreservedPassIDs.count(*I))
        PreservedPassIDs.erase(*I);
      I = Next;
    }

Or, with appropriate API:

    iterator erase(const_iterator I);

to:

    // Better guarantee; no lookup required.
    for (auto I = PreservedPassIDs.begin(), E = PreservedPassIDs.end();
         I != E;)
      if (!Arg.PreservedPassIDs.count(*I))
        I = PreservedPassIDs.erase(I);
      else
        ++I;

I just looked into `SmallPtrSet` to see how hard this API would be to
implement (see the attached patch), and actually neither guarantee is
kept right now when it's in small mode.  Have a look at the FIXME in
`eraseImplSmall()` in the patch I attached to see why: SmallPtrSet swaps
the last valid bucket in place of the erased element, keeping all
non-empty buckets at the beginning (I assume to make successful linear
searches faster).  It does keep a weaker guarantee: `erase()` will only
invalidate iterators of successors to the erased element.

So it turns out your code has a bug (which I'd missed before), since
incrementing will *skip* the next element when `SmallPtrSet` is in
"small" mode.  The new API would still work though:

    // Weak guarantee; no lookup required.
    for (auto I = PreservedPassIDs.begin(), E = PreservedPassIDs.end();
         I != E;)
      if (!Arg.PreservedPassIDs.count(*I))
        I = PreservedPassIDs.erase(I);
      else
        ++I;

> > +  /// \brief Query whether a pass is marked as preserved by this set.
> > +  template <typename PassT> bool preserved() const {
> > +    return preserved(PassT::ID());
> > +  }
> > +
> > +  /// \brief Query whether an abstract pass ID is marked as preserved by this
> > +  /// set.
> > +  bool preserved(void *PassID) const {
> > +    return PreservedPassIDs.count((void *)AllPassesID) ||
> > +           PreservedPassIDs.count(PassID);
> > +  }
> 
> Shouldn't these functions be spelled "isPreserved()"?
> 
> They both read fine to me, and so I picked the shorter. In particular, there isn't the ambiguity of "empty" the verb vs "empty" the state. But if folks would rather just use 'is' everywhere, I can live with that too.

I think I'd read `preserved()` as a getter that gave the set of
preserved pass ids (short for `preservedPassIDs()` or something).

I don't really have a problem with it, but it does violate the style
guide [1] ("function names should be verb phrases (as they represent
actions)") since "preserved" isn't a verb phrase.

[1]: http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly


-------------- next part --------------
A non-text attachment was scrubbed...
Name: smallptrset-erase-by-iterator.patch
Type: application/octet-stream
Size: 3653 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150107/e755ab3b/attachment.obj>