Revert "[TableGen] Use heap allocated arrays instead of vectors for TreePatternNode::Types and ResultPerm. NFC"

[tmatheson]

Summary

While working on DAGISelMatcherEmitter I've hit several runtime errors
caused by accessing TreePatternNode::Types out of bounds. These were
difficult to debug because the switch from std::vector to unique_ptr
removes bounds checking.

I don't think the slight reduction in class size is worth the extra
debugging and memory safety problems, so I suggest we revert this.

This reverts commit d34125a1a825208b592cfa8f5fc3566303d691a4.

[craig.topper]

Lgtm

[craig.topper]

Alternatively, could we add bounds checking to getExtType and make more use of it?

[tmatheson]

Thanks, I've done a complete revert for simplicity, but I do think that using getNumTypes() etc rather than Types.size() was nicer.

My concern with adding the bounds checking to getExtType would be that the class is already quite sprawling, and it would only be a matter of time before someone used Types directly and raised the same problems. Ideally whatever does the bounds checking should be encapsulated well so that we can be confident it is correct. std::vector accomplishes this already.

I'm not sure what the performance bottlenecks are here but maybe avoiding the allocation in most cases with SmallVector is something to consider too. Or combining the two vectors, since they should always be the same length.

llvm/utils/TableGen/CodeGenDAGPatterns.h

bool operator==(const TreePredicateCall &o) const {
     return Fn == o.Fn && Scope == o.Scope;
   }
   bool operator!=(const TreePredicateCall &o) const {
     return !(*this == o);
   }
 };
 
 class TreePatternNode : public RefCountedBase<TreePatternNode> {
-  /// Number of results for this node.
-  unsigned NumResults;
-
   /// The type of each node result.  Before and during type inference, each
   /// result may be a set of possible types.  After (successful) type inference,
   /// each is a single concrete type.
-  std::unique_ptr<TypeSetByHwMode[]> Types;
+  std::vector<TypeSetByHwMode> Types;
 
   /// The index of each result in results of the pattern.
-  std::unique_ptr<unsigned[]> ResultPerm;
+  std::vector<unsigned> ResultPerm;
 
   /// OperatorOrVal - The Record for the operator if this is an interior node
   /// (not a leaf) or the init value (e.g. the "GPRC" record, or "7") for a
   /// leaf.
   PointerUnion<Record *, Init *> OperatorOrVal;
 
   /// Name - The name given to this node with the :$foo notation.
   ///
   std::string Name;
 
   std::vector<ScopedName> NamesAsPredicateArg;
 
   /// PredicateCalls - The predicate functions to execute on this node to check
   /// for a match.  If this list is empty, no predicate is involved.
   std::vector<TreePredicateCall> PredicateCalls;
 
   /// TransformFn - The transformation function to execute on this node before
   /// it can be substituted into the resulting instruction on a pattern match.
-  Record *TransformFn = nullptr;
+  Record *TransformFn;
 
   std::vector<TreePatternNodePtr> Children;
 
   /// If this was instantiated from a PatFrag node, and the PatFrag was derived
   /// from "GISelFlags": the original Record derived from GISelFlags.
   const Record *GISelFlags = nullptr;
 
 public:
   TreePatternNode(Record *Op, std::vector<TreePatternNodePtr> Ch,
-                  unsigned numResults)
-      : NumResults(numResults), Types(new TypeSetByHwMode[numResults]),
-        ResultPerm(new unsigned[numResults]), OperatorOrVal(Op),
-        Children(std::move(Ch)) {
-    std::iota(ResultPerm.get(), ResultPerm.get() + numResults, 0);
-  }
-  TreePatternNode(Init *val, unsigned numResults) // leaf ctor
-      : NumResults(numResults), Types(new TypeSetByHwMode[numResults]),
-        ResultPerm(new unsigned[numResults]), OperatorOrVal(val) {
-    std::iota(ResultPerm.get(), ResultPerm.get() + numResults, 0);
+                  unsigned NumResults)
+      : OperatorOrVal(Op), TransformFn(nullptr), Children(std::move(Ch)) {
+    Types.resize(NumResults);
+    ResultPerm.resize(NumResults);
+    std::iota(ResultPerm.begin(), ResultPerm.end(), 0);
+  }
+  TreePatternNode(Init *val, unsigned NumResults) // leaf ctor
+      : OperatorOrVal(val), TransformFn(nullptr) {
+    Types.resize(NumResults);
+    ResultPerm.resize(NumResults);
+    std::iota(ResultPerm.begin(), ResultPerm.end(), 0);
   }
 
   bool hasName() const { return !Name.empty(); }
   const std::string &getName() const { return Name; }
   void setName(StringRef N) { Name.assign(N.begin(), N.end()); }
 
   const std::vector<ScopedName> &getNamesAsPredicateArg() const {
     return NamesAsPredicateArg;
   }
   void setNamesAsPredicateArg(const std::vector<ScopedName>& Names) {
     NamesAsPredicateArg = Names;
   }
   void addNameAsPredicateArg(const ScopedName &N) {
     NamesAsPredicateArg.push_back(N);
   }
 
   bool isLeaf() const { return isa<Init *>(OperatorOrVal); }
 
   // Type accessors.
-  unsigned getNumTypes() const { return NumResults; }
+  unsigned getNumTypes() const { return Types.size(); }
   ValueTypeByHwMode getType(unsigned ResNo) const {
     return Types[ResNo].getValueTypeByHwMode();
   }
-  ArrayRef<TypeSetByHwMode> getExtTypes() const {
-    return ArrayRef(Types.get(), NumResults);
-  }
-  MutableArrayRef<TypeSetByHwMode> getExtTypes() {
-    return MutableArrayRef(Types.get(), NumResults);
-  }
+  const std::vector<TypeSetByHwMode> &getExtTypes() const { return Types; }
   const TypeSetByHwMode &getExtType(unsigned ResNo) const {
     return Types[ResNo];
   }
   TypeSetByHwMode &getExtType(unsigned ResNo) { return Types[ResNo]; }
   void setType(unsigned ResNo, const TypeSetByHwMode &T) { Types[ResNo] = T; }
   MVT::SimpleValueType getSimpleType(unsigned ResNo) const {
     return Types[ResNo].getMachineValueType().SimpleTy;
   }
 
   bool hasConcreteType(unsigned ResNo) const {
     return Types[ResNo].isValueTypeByHwMode(false);
   }
   bool isTypeCompletelyUnknown(unsigned ResNo, TreePattern &TP) const {
     return Types[ResNo].empty();
   }
 
+  unsigned getNumResults() const { return ResultPerm.size(); }
   unsigned getResultIndex(unsigned ResNo) const { return ResultPerm[ResNo]; }
   void setResultIndex(unsigned ResNo, unsigned RI) { ResultPerm[ResNo] = RI; }
 
   Init *getLeafValue() const {
     assert(isLeaf());
     return cast<Init *>(OperatorOrVal);
   }
   Record *getOperator() const {

llvm/utils/TableGen/CodeGenDAGPatterns.cpp

else if (Operand->isSubClassOf("RegisterOperand"))
     RC = Operand->getValueAsDef("RegClass");
 
   assert(RC && "Unknown operand type");
   CodeGenTarget &Tgt = TP.getDAGPatterns().getTargetInfo();
   return UpdateNodeType(ResNo, Tgt.getRegisterClass(RC).getValueTypes(), TP);
 }
 
 bool TreePatternNode::ContainsUnresolvedType(TreePattern &TP) const {
-  for (unsigned i = 0, e = getNumTypes(); i != e; ++i)
+  for (unsigned i = 0, e = Types.size(); i != e; ++i)
     if (!TP.getInfer().isConcrete(Types[i], true))
       return true;
   for (unsigned i = 0, e = getNumChildren(); i != e; ++i)
     if (getChild(i)->ContainsUnresolvedType(TP))
       return true;
   return false;
 }
 
 bool TreePatternNode::hasProperTypeByHwMode() const {
-  for (const TypeSetByHwMode &S : getExtTypes())
+  for (const TypeSetByHwMode &S : Types)
     if (!S.isSimple())
       return true;
   for (const TreePatternNodePtr &C : Children)
     if (C->hasProperTypeByHwMode())
       return true;
   return false;
 }
 
 bool TreePatternNode::hasPossibleType() const {
-  for (const TypeSetByHwMode &S : getExtTypes())
+  for (const TypeSetByHwMode &S : Types)
     if (!S.isPossible())
       return false;
   for (const TreePatternNodePtr &C : Children)
     if (!C->hasPossibleType())
       return false;
   return true;
 }
 
 bool TreePatternNode::setDefaultMode(unsigned Mode) {
-  for (TypeSetByHwMode &S : getExtTypes()) {
+  for (TypeSetByHwMode &S : Types) {
     S.makeSimple(Mode);
     // Check if the selected mode had a type conflict.
     if (S.get(DefaultMode).empty())
       return false;
   }
   for (const TreePatternNodePtr &C : Children)
     if (!C->setDefaultMode(Mode))
       return false;

 }
 
 void TreePatternNode::print(raw_ostream &OS) const {
   if (isLeaf())
     OS << *getLeafValue();
   else
     OS << '(' << getOperator()->getName();
 
-  for (unsigned i = 0, e = getNumTypes(); i != e; ++i) {
+  for (unsigned i = 0, e = Types.size(); i != e; ++i) {
     OS << ':';
     getExtType(i).writeToStream(OS);
   }
 
   if (!isLeaf()) {
     if (getNumChildren() != 0) {
       OS << " ";
       ListSeparator LS;
if (isLeaf()) {
     CChildren.reserve(Children.size());
     for (unsigned i = 0, e = getNumChildren(); i != e; ++i)
       CChildren.push_back(getChild(i)->clone());
     New = makeIntrusiveRefCnt<TreePatternNode>(
         getOperator(), std::move(CChildren), getNumTypes());
   }
   New->setName(getName());
   New->setNamesAsPredicateArg(getNamesAsPredicateArg());
-  llvm::copy(getExtTypes(), New->getExtTypes().begin());
+  New->Types = Types;
   New->setPredicateCalls(getPredicateCalls());
   New->setGISelFlagsRecord(getGISelFlagsRecord());
   New->setTransformFn(getTransformFn());
   return New;
 }
 
 /// RemoveAllTypes - Recursively strip all the types of this tree.
 void TreePatternNode::RemoveAllTypes() {
   // Reset to unknown type.
-  std::fill(getExtTypes().begin(), getExtTypes().end(), TypeSetByHwMode());
+  std::fill(Types.begin(), Types.end(), TypeSetByHwMode());
   if (isLeaf()) return;
   for (unsigned i = 0, e = getNumChildren(); i != e; ++i)
     getChild(i)->RemoveAllTypes();
 }
 
 
 /// SubstituteFormalArguments - Replace the formal arguments in this tree
 /// with actual values specified by ArgMap.
do {
           getOperator(), std::move(NewChildren), getNumTypes());
 
       // Copy over properties.
       R->setName(getName());
       R->setNamesAsPredicateArg(getNamesAsPredicateArg());
       R->setPredicateCalls(getPredicateCalls());
       R->setGISelFlagsRecord(getGISelFlagsRecord());
       R->setTransformFn(getTransformFn());
-      for (unsigned i = 0, e = getNumTypes(); i != e; ++i) {
+      for (unsigned i = 0, e = getNumTypes(); i != e; ++i)
         R->setType(i, getExtType(i));
+      for (unsigned i = 0, e = getNumResults(); i != e; ++i)
         R->setResultIndex(i, getResultIndex(i));
-      }
 
       // Register alternative.
       OutAlternatives.push_back(R);
 
       // Increment indices to the next permutation by incrementing the
       // indices from last index backward, e.g., generate the sequence
       // [0, 0], [0, 1], [1, 0], [1, 1].
       int IdxsIdx;
bool TreePatternNode::ApplyTypeConstraints(TreePattern &TP, bool NotRegisters) {
   if (TP.hasError())
     return false;
 
   CodeGenDAGPatterns &CDP = TP.getDAGPatterns();
   if (isLeaf()) {
     if (DefInit *DI = dyn_cast<DefInit>(getLeafValue())) {
       // If it's a regclass or something else known, include the type.
       bool MadeChange = false;
-      for (unsigned i = 0, e = getNumTypes(); i != e; ++i)
+      for (unsigned i = 0, e = Types.size(); i != e; ++i)
         MadeChange |= UpdateNodeType(i, getImplicitType(DI->getDef(), i,
                                                         NotRegisters,
                                                         !hasName(), TP), TP);
       return MadeChange;
     }
 
     if (IntInit *II = dyn_cast<IntInit>(getLeafValue())) {
-      assert(getNumTypes() == 1 && "Invalid IntInit");
+      assert(Types.size() == 1 && "Invalid IntInit");
 
       // Int inits are always integers. :)
       bool MadeChange = TP.getInfer().EnforceInteger(Types[0]);
 
       if (!TP.getInfer().isConcrete(Types[0], false))
         return MadeChange;
 
       ValueTypeByHwMode VVT = TP.getInfer().getConcrete(Types[0], false);
for (unsigned i = 0, e = getNumChildren(); i != e; ++i)
       MadeChange |= getChild(i)->ApplyTypeConstraints(TP, NotRegisters);
     return MadeChange;
   }
 
   if (getOperator()->isSubClassOf("ComplexPattern")) {
     bool MadeChange = false;
 
     if (!NotRegisters) {
-      assert(getNumTypes() == 1 && "ComplexPatterns only produce one result!");
+      assert(Types.size() == 1 && "ComplexPatterns only produce one result!");
       Record *T = CDP.getComplexPattern(getOperator()).getValueType();
       const CodeGenHwModes &CGH = CDP.getTargetInfo().getHwModes();
       const ValueTypeByHwMode VVT = getValueTypeByHwMode(T, CGH);
       // TODO: AArch64 and AMDGPU use ComplexPattern<untyped, ...> and then
       // exclusively use those as non-leaf nodes with explicit type casts, so
       // for backwards compatibility we do no inference in that case. This is
       // not supported when the ComplexPattern is used as a leaf value,
       // however; this inconsistency should be resolved, either by adding this

llvm/utils/TableGen/DAGISelMatcherGen.cpp

for (Record *Reg : Pattern.getDstRegs()) {
       if (!Reg->isSubClassOf("Register") || Reg == HandledReg) continue;
       ++NumSrcResults;
     }
   }
 
   SmallVector<unsigned, 8> Results(Ops);
 
   // Apply result permutation.
-  for (unsigned ResNo = 0; ResNo < Pattern.getDstPattern()->getNumTypes();
+  for (unsigned ResNo = 0; ResNo < Pattern.getDstPattern()->getNumResults();
        ++ResNo) {
     Results[ResNo] = Ops[Pattern.getDstPattern()->getResultIndex(ResNo)];
   }
 
   Results.resize(NumSrcResults);
   AddMatcher(new CompleteMatchMatcher(Results, Pattern));
 }