[llvm] r178319 - Revert r178166. According to Howard, this code is actually ok.
David Blaikie
dblaikie at gmail.com
Wed Apr 17 16:51:14 PDT 2013
On Wed, Apr 17, 2013 at 7:25 AM, Howard Hinnant <hhinnant at apple.com> wrote:
>
> 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.
Thanks for the breakdown, Howard - we'll have to follow up internally
to get a better sense of the basis for the bug report. If it's a
scenario that we should be caring about (rather than picking up a
bugfix for whatever c++ library we're using, etc) then we'll certainly
take your advice into consideration about how best to re-apply the
fix. I'm happy to leave it out for now/until we can justify it more
clearly.
> 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.
Good point about these data structures in general - could be
interesting to see what perf numbers for such a change look like. I
don't know how important/oft-used these data structures are so I
couldn't really speculate about whether/how much it matters.
>
> 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
>
More information about the llvm-commits
mailing list