[TableGen] Fix bug in TableGen CodeGenPatterns when adding variants of the patterns.

[aymanmus]

Summary

All patterns reside in a std::vector container, where new variants are added to it using the standard library's emplace_back function.
When calling this with a new element while there is no enough allocated space, a bigger space is allocated and all the old info in the small vector is copied to the newly allocated vector, then the old vector is freed.
The problem is that before doing this "copying", we take a reference of one of the elements in the old vector, and after the "copying" we add it to the new vector.
As the old vector is freed after the copying, the reference now does not point to a valid element.

Added new function to the API of CodeGenDAGPatterns class to return the same information as a copy in order to avoid this issue.

This was revealed in rL305465 that added many patterns and forced the reallocation of the vector which caused crashes in windows bots.

[RKSimon]

Any chance of a test case?

[aymanmus]

@RKSimon, when I recommit my patch in D33188 the build will crash on windows without this.

[craig.topper]

Can we use add a move constructor to PatternToMatch, and use push_back(PatternToMatch(PatternsToMatch[i].getSrcRecord(), ....)? That's the right way to fix this I believe.

[aymanmus]

The only problematic thing here is that DstRegs was returned as reference, the rest of the class' elements are either pointers or integer types.
I think using a copy constructor instead of only copying the needed element is kind of an overhead, don't you agree?

P.s. from a fast experiment I did, I noticed there is a slight increase in build time in tablegen phase when using copy constructor.

[craig.topper]

We don't need a new method to copy a vector. The caller can always make a copy into a local variable first.

What we should try to do is avoid inserting an extra copy of the vector. What we should try to do is make the copy occur before the vector is resized not after.

I proposed adding a move constructor not a copy constructor so that calling push_back(PatternToMatch( will move the object into place after creating the object. This way the copy happens before and only moves happen after the vector is resized.

An alternative is to capture the DstRegs in a local variable and std::move it to the emplace_back call. Then we also need to change the signature of the constructor to take the vector by value and then std::move it into the class. This would require an audit and possible changes at all other uses of the constructor.

I think adding the move constructor is probably the simpler option.

[craig.topper]

LGTM

llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp

for (unsigned v = 0, e = Variants.size(); v != e; ++v) {
           AlreadyExists = true;
           break;
         }
       }
       // If we already have it, ignore the variant.
       if (AlreadyExists) continue;
 
       // Otherwise, add it to the list of patterns we have.
-      PatternsToMatch.emplace_back(
+      PatternsToMatch.push_back(PatternToMatch(
           PatternsToMatch[i].getSrcRecord(), PatternsToMatch[i].getPredicates(),
           Variant, PatternsToMatch[i].getDstPattern(),
           PatternsToMatch[i].getDstRegs(),
-          PatternsToMatch[i].getAddedComplexity(), Record::getNewUID());
+          PatternsToMatch[i].getAddedComplexity(), Record::getNewUID()));
     }
 
     DEBUG(errs() << "\n");
   }
 }