[TableGen] Make DAGInstruction own Pattern to avoid leaking it.

[nhaehnle]

utils/TableGen/CodeGenDAGPatterns.h

924–925

Why do you need a non-const overload here? The pointer itself should never be changed.

Actually, it would arguably be nicer to just return a reference here, since the fact that the TreePattern is stored in a unique_ptr seems like an implementation detail that callers should not have to worry about.

[fhahn]

Thanks for having a look. Dropped the non-const accessor.

[fhahn]

utils/TableGen/CodeGenDAGPatterns.h

924–925

Actually, it would arguably be nicer to just return a reference here, since the fact that the TreePattern is stored in a unique_ptr seems like an implementation detail that callers should not have to worry about.

There are some places that check that the pattern is not null, before accessing the value. Having a unique_ptr there seemed just easier. But I suppose we could have something like hasPattern, if you think that would be better?

[dsanders]

utils/TableGen/CodeGenDAGPatterns.h

924

This code looks like it's either making a copy of pattern or giving ownership of DAGInstruction::Pattern to the caller. Shouldn't it be returning a raw pointer?

[fhahn]

Thanks, updated to return TreePattern*.

[fhahn]

ping

[craig.topper]

utils/TableGen/CodeGenDAGPatterns.cpp

3575

Why doesn't insert work? There should be an rvalue pair version.

[fhahn]

Do not create pair explicitly with emplace.

utils/TableGen/CodeGenDAGPatterns.cpp

3575

Thanks. With emplace we do not have to use make_pair.

[craig.topper]

LGTM

utils/TableGen/CodeGenDAGPatterns.h

 
 /// DAGDefaultOperand - One of these is created for each OperandWithDefaultOps
 /// that has a set ExecuteAlways / DefaultOps field.
 struct DAGDefaultOperand {
   std::vector<TreePatternNodePtr> DefaultOps;
 };
 
 class DAGInstruction {
-  TreePattern *Pattern;
+  std::unique_ptr<TreePattern> Pattern;
   std::vector<Record*> Results;
   std::vector<Record*> Operands;
   std::vector<Record*> ImpResults;
   TreePatternNodePtr ResultPattern;
 
 public:
-  DAGInstruction(TreePattern *TP,
+  DAGInstruction(std::unique_ptr<TreePattern> &&TP,
                  const std::vector<Record*> &results,
                  const std::vector<Record*> &operands,
                  const std::vector<Record*> &impresults)
-    : Pattern(TP), Results(results), Operands(operands),
+    : Pattern(std::move(TP)), Results(results), Operands(operands),
       ImpResults(impresults), ResultPattern(nullptr) {}
 
-  TreePattern *getPattern() const { return Pattern; }
+  const std::unique_ptr<TreePattern> &getPattern() const { return Pattern; }
   unsigned getNumResults() const { return Results.size(); }
   unsigned getNumOperands() const { return Operands.size(); }
   unsigned getNumImpResults() const { return ImpResults.size(); }
   const std::vector<Record*>& getImpResults() const { return ImpResults; }
 
   void setResultPattern(TreePatternNodePtr R) { ResultPattern = R; }
 
   Record *getResult(unsigned RN) const {
     assert(RN < Results.size());

utils/TableGen/CodeGenDAGPatterns.cpp

public:
   bool mayLoad;
   bool isBitcast;
   bool isVariadic;
 
   InstAnalyzer(const CodeGenDAGPatterns &cdp)
     : CDP(cdp), hasSideEffects(false), mayStore(false), mayLoad(false),
       isBitcast(false), isVariadic(false) {}
 
-  void Analyze(const TreePattern *Pat) {
+  void Analyze(const std::unique_ptr<TreePattern> &Pat) {
     // Assume only the first tree is the pattern. The others are clobber nodes.
     AnalyzeNode(Pat->getTree(0).get());
   }
 
   void Analyze(const PatternToMatch &Pat) {
     AnalyzeNode(Pat.getSrcPattern());
   }
 

 }
 
 const DAGInstruction &CodeGenDAGPatterns::parseInstructionPattern(
     CodeGenInstruction &CGI, ListInit *Pat, DAGInstMap &DAGInsts) {
 
   assert(!DAGInsts.count(CGI.TheDef) && "Instruction already parsed!");
 
   // Parse the instruction.
-  TreePattern *I = new TreePattern(CGI.TheDef, Pat, true, *this);
+  auto I = llvm::make_unique<TreePattern>(CGI.TheDef, Pat, true, *this);
   // Inline pattern fragments into it.
   I->InlinePatternFragments();
 
   // Infer as many types as possible.  If we cannot infer all of them, we can
   // never do anything with this instruction pattern: report it to the user.
   if (!I->InferAllTypes())
     I->error("Could not infer all types in pattern!");
 
if (Pat->getNumTypes() != 0) {
         Pat->getExtType(k).writeToStream(OS);
       }
       I->error("Top-level forms in instruction pattern should have"
                " void types, has types " +
                OS.str());
     }
 
     // Find inputs and outputs, and verify the structure of the uses/defs.
-    FindPatternInputsAndOutputs(I, Pat, InstInputs, InstResults,
+    FindPatternInputsAndOutputs(I.get(), Pat, InstInputs, InstResults,
                                 InstImpResults);
   }
 
   // Now that we have inputs and outputs of the pattern, inspect the operands
   // list for the instruction.  This determines the order that operands are
   // added to the machine instruction the node corresponds to.
   unsigned NumResults = InstResults.size();
 
const DAGInstruction &CodeGenDAGPatterns::parseInstructionPattern(
   // Copy fully inferred output node types to instruction result pattern.
   for (unsigned i = 0; i != NumResults; ++i) {
     assert(ResNodes[i]->getNumTypes() == 1 && "FIXME: Unhandled");
     ResultPattern->setType(i, ResNodes[i]->getExtType(0));
   }
 
   // Create and insert the instruction.
   // FIXME: InstImpResults should not be part of DAGInstruction.
-  DAGInstruction TheInst(I, Results, Operands, InstImpResults);
-  DAGInsts.insert(std::make_pair(I->getRecord(), TheInst));
+  TreePattern *InstPattern = I.get();
+  DAGInstruction TheInst(std::move(I), Results, Operands, InstImpResults);
+  DAGInsts.emplace(
+      std::make_pair(InstPattern->getRecord(), std::move(TheInst)));
 
   // Use a temporary tree pattern to infer all types and make sure that the
   // constructed result is correct.  This depends on the instruction already
   // being inserted into the DAGInsts map.
-  TreePattern Temp(I->getRecord(), ResultPattern, false, *this);
-  Temp.InferAllTypes(&I->getNamedNodesMap());
+  TreePattern Temp(InstPattern->getRecord(), ResultPattern, false, *this);
+  Temp.InferAllTypes(&InstPattern->getNamedNodesMap());
 
-  DAGInstruction &TheInsertedInst = DAGInsts.find(I->getRecord())->second;
+  DAGInstruction &TheInsertedInst =
+      DAGInsts.find(InstPattern->getRecord())->second;
   TheInsertedInst.setResultPattern(Temp.getOnlyTree());
 
   return TheInsertedInst;
 }
 
 /// ParseInstructions - Parse all of the instructions, inlining and resolving
 /// any fragments involved.  This populates the Instructions list with fully
 /// resolved instructions.
for (Record *Instr : Instrs) {
 
     (void)DI;
     LLVM_DEBUG(DI.getPattern()->dump());
   }
 
   // If we can, convert the instructions to be patterns that are matched!
   for (auto &Entry : Instructions) {
     DAGInstruction &TheInst = Entry.second;
-    TreePattern *I = TheInst.getPattern();
+    const std::unique_ptr<TreePattern> &I = TheInst.getPattern();
     if (!I) continue;  // No pattern.
 
     if (PatternRewriter)
-      PatternRewriter(I);
+      PatternRewriter(I.get());
     // FIXME: Assume only the first tree is the pattern. The others are clobber
     // nodes.
     TreePatternNodePtr Pattern = I->getTree(0);
     TreePatternNodePtr SrcPattern;
     if (Pattern->getOperator()->getName() == "set") {
       SrcPattern = Pattern->getChild(Pattern->getNumChildren()-1)->clone();
     } else{
       // Not a set (store or something?)
       SrcPattern = Pattern;
     }
 
     Record *Instr = Entry.first;
     ListInit *Preds = Instr->getValueAsListInit("Predicates");
     int Complexity = Instr->getValueAsInt("AddedComplexity");
     AddPatternToMatch(
-        I,
+        I.get(),
         PatternToMatch(Instr, makePredList(Preds), SrcPattern,
                        TheInst.getResultPattern(), TheInst.getImpResults(),
                        Complexity, Instr->getID()));
   }
 }
 
 typedef std::pair<TreePatternNode *, unsigned> NameRecord;
 
void CodeGenDAGPatterns::InferInstructionFlags() {
   // First try to infer flags from the primary instruction pattern, if any.
   SmallVector<CodeGenInstruction*, 8> Revisit;
   unsigned Errors = 0;
   for (unsigned i = 0, e = Instructions.size(); i != e; ++i) {
     CodeGenInstruction &InstInfo =
       const_cast<CodeGenInstruction &>(*Instructions[i]);
 
     // Get the primary instruction pattern.
-    const TreePattern *Pattern = getInstruction(InstInfo.TheDef).getPattern();
+    const std::unique_ptr<TreePattern> &Pattern =
+        getInstruction(InstInfo.TheDef).getPattern();
     if (!Pattern) {
       if (InstInfo.hasUndefFlags())
         Revisit.push_back(&InstInfo);
       continue;
     }
     InstAnalyzer PatInfo(*this);
     PatInfo.Analyze(Pattern);
     Errors += InferFromPattern(InstInfo, PatInfo, InstInfo.TheDef);

utils/TableGen/DAGISelMatcherGen.cpp

void MatcherGen::EmitResultLeafAsOperand(const TreePatternNode *N,
   errs() << "unhandled leaf node: \n";
   N->dump();
 }
 
 /// GetInstPatternNode - Get the pattern for an instruction.
 ///
 const TreePatternNode *MatcherGen::
 GetInstPatternNode(const DAGInstruction &Inst, const TreePatternNode *N) {
-  const TreePattern *InstPat = Inst.getPattern();
+  const std::unique_ptr<TreePattern> &InstPat = Inst.getPattern();
 
   // FIXME2?: Assume actual pattern comes before "implicit".
   TreePatternNode *InstPatNode;
   if (InstPat)
     InstPatNode = InstPat->getTree(0).get();
   else if (/*isRoot*/ N == Pattern.getDstPattern())
     InstPatNode = Pattern.getSrcPattern();
   else