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

[nhaehnle]

utils/TableGen/CodeGenDAGPatterns.h

924–925 ↗

(On Diff #149069)

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 ↗

(On Diff #149069)

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 ↗

(On Diff #149086)

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 ↗

(On Diff #149156)

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 ↗

(On Diff #149156)

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

[craig.topper]

LGTM

llvm/trunk/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; }
+  TreePattern *getPattern() const { return Pattern.get(); }
   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 {

llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp

 }
 
 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(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.