[llvm] r178319 - Revert r178166. According to Howard, this code is actually ok.

[Howard Hinnant]

On Apr 17, 2013, at 8:33 AM, David Blaikie <dblaikie at gmail.com> wrote:

> 
> On Mar 29, 2013 11:16 AM, "Dan Gohman" <dan433584 at gmail.com> wrote:
> >
> > Author: djg
> > Date: Thu Mar 28 19:13:08 2013
> > New Revision: 178319
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=178319&view=rev
> > Log:
> > Revert r178166. According to Howard, this code is actually ok.
> 
> Well. It's technically correct according to the standard, but do we know how well the standard is implemented in the implementations wee care about? I believe this was found as a bug when using at least one version of libstdc++...

I started investigating this.  This file came into existence in 2012.  I'm quite sure that gcc-4.0 (2006) nor gcc-4.2 (2007) had this bug.  It could have been introduced after one of those two versions, I'm not sure.

In order for a vector implementation to exhibit such a bug it would have to:

1.  Allocate a new buffer.
2.  Copy/move everything from the old buffer to the new buffer.
3.  Destruct/deallocate the old buffer.
4.  Construct the new element in the new buffer.

This implementation would not only cause the self-referencing bug, it would also not meet the exception safety guarantee specified by the standard:  If an exception is thrown by push_back, there shall be no effects.  This exception safety specification dates back to C++98.

One possibility is that the bug was introduced with the introduction of move semantics, and I note that PredTransition has an implicitly generated move constructor (if compiled with a 2011 or later compiler in -std=c++11):

1.  Allocate a new buffer.
2.  Move everything from the old buffer to the new buffer.
3.  Construct the new element in the new buffer.
4.  Destruct/deallocate the old buffer.

This is quite possible as this would have been the natural implementation of push_back (the realloc branch) for C++98/03.  In C++11 the above algorithm also fails to meet the C++98 exception safety guarantee because if 3) throws, the move in 2) has altered the old buffer.  Though I guess you could catch the exception and move everything back if you have a noexcept move assignment operator.

If one wants to protect against such a std::vector bug, I recommend:

   TransVec.push_back(PredTransition(TransVec[TransIdx]));

This is better than the original workaround of:

    PredTransition Trans = TransVec[TransIdx];
    TransVec.push_back(Trans);

Because the latter costs 2 copy constructions whereas the former costs 1 copy construction and 1 move construction.

If you don't need to work around such a std::vector bug:

   TransVec.push_back(TransVec[TransIdx]);

the cost is 1 copy construction.

While investigating this I took a look at PredTransition:

struct PredCheck {
  bool IsRead;
  unsigned RWIdx;
  Record *Predicate;

  PredCheck(bool r, unsigned w, Record *p): IsRead(r), RWIdx(w), Predicate(p) {}
};

// A Predicate transition is a list of RW sequences guarded by a PredTerm.
struct PredTransition {
  // A predicate term is a conjunction of PredChecks.
  SmallVector<PredCheck, 4> PredTerm;
  SmallVector<SmallVector<unsigned,4>, 16> WriteSequences;
  SmallVector<SmallVector<unsigned,4>, 16> ReadSequences;
  SmallVector<unsigned, 4> ProcIndices;
};

On a 64 bit platform, sizeof(PredTransition) is 1.4Kb.  The reason that number is important is because the cost of a move construction or move assignment is in general directly proportional to sizeof.  And when you do vector<PredTransition>, you are inviting a great many move constructions and move assignments of PredTransition.

If one were to substitute std::vector for SmallVector in PredTransition, then sizeof(PredTransition) drops from 1.4Kb to 0.09Kb, or about 15X smaller, and consequently, move operations that are about 15X faster.

Disclaimer:  I have not done any timings here.  This is all back-of-the-envelope estimations.  However in this particular use case, I suspect that SmallVector may be a significant pessimization, instead of an optimization.  Only by measuring would one know for sure.

Howard

> >
> > Modified:
> >     llvm/trunk/utils/TableGen/CodeGenSchedule.cpp
> >
> > Modified: llvm/trunk/utils/TableGen/CodeGenSchedule.cpp
> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGen/CodeGenSchedule.cpp?rev=178319&r1=178318&r2=178319&view=diff
> > ==============================================================================
> > --- llvm/trunk/utils/TableGen/CodeGenSchedule.cpp (original)
> > +++ llvm/trunk/utils/TableGen/CodeGenSchedule.cpp Thu Mar 28 19:13:08 2013
> > @@ -1105,9 +1105,7 @@ void PredTransitions::getIntersectingVar
> >        // Push another copy of the current transition for more variants.
> >        Variant.TransVecIdx = TransVec.size();
> >        IntersectingVariants.push_back(Variant);
> > -
> > -      PredTransition Trans = TransVec[TransIdx];
> > -      TransVec.push_back(Trans);
> > +      TransVec.push_back(TransVec[TransIdx]);
> >      }
> >    }
> >  }
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits