[PATCH] D47463: [TableGen] Avoid leaking TreePatternNodes by using shared_ptr.
David Blaikie via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 4 13:24:47 PDT 2018
dblaikie added a comment.
Great to see these sort of improvements to memory ownership!
Just a few general pointers (I went through and gave specific feedback in some instances, but it was a bit repetitive):
- Some std::move opportunities
- Some opportunities to avoid copying smart pointers (avoid refcount increment/decrement)
- Some places where APIs could be change to take Nodes by ref or const ref (rather than pointer or smart pointer) that would simplify the callers (& incidentally make the callers identical between smart and plain pointers - "*X" in both cases)
Also, I think it may be better not to typedef the smart pointer type - it looks like it makes it easier to accidentally copy them when that may not be desirable. I'd keep it as an explicit feature of the type name (rather than behind a typedef) & try to reduce the number of places that need to name the shared_ptr type to only those that are really handling ownership transfer, etc.
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp:2522
+ for (TreePatternNodePtr &Tree : Trees)
+ ComputeNamedNodes(Tree.get());
}
----------------
Maybe ComputeNamedNodes could take by Node reference instead of by Node pointer? (then the call site would be "ComputeNamedNodes(*Tree);" - so it'd match/work fine with either plain pointers or smart pointers)
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp:2674-2676
+ TreePatternNodePtr IIDNode =
+ std::make_shared<TreePatternNode>(IntInit::get(IID), 1);
Children.insert(Children.begin(), IIDNode);
----------------
auto & std::move? Or maybe just inline it all together:
Children.insert(Children.begin(), std::make_shared<TreePatternNode>(IntInit::get(IID), 1));
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp:2681
for (unsigned i = 0; i < Children.size(); ++i) {
- TreePatternNode *Child = Children[i];
+ TreePatternNodePtr Child = Children[i];
----------------
Node reference (eg: "auto &Child = *Children[i];") maybe? Doesn't look like this needs ownership?
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp:2700
- TreePatternNode *Result = new TreePatternNode(Operator, Children, NumResults);
+ TreePatternNodePtr Result =
+ std::make_shared<TreePatternNode>(Operator, Children, NumResults);
----------------
auto, maybe
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp:2737
MadeChange |= SimplifyTree(Child);
N->setChild(i, Child);
}
----------------
std::move?
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp:3096
if (!Slot) {
Slot = Pat;
return true;
----------------
If this API continues to take Pat by value, this should std::move here - though if this move only happens in a relative minority of calls, perhaps it should take the parameter by const ref instead of incurring the ref count handling for every call?
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp:3448
TypesString.clear();
- TreePatternNode *Pat = I->getTree(j);
+ TreePatternNodePtr Pat = I->getTree(j);
if (Pat->getNumTypes() != 0) {
----------------
Node reference here, doesn't look like this needs any ownership?
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp:3488
ResNodes.push_back(RNode);
----------------
std::move? (after moving this down past the one other use of RNode)
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp:3529
}
- TreePatternNode *InVal = InstInputsCheck[OpName];
+ TreePatternNodePtr InVal = InstInputsCheck[OpName];
InstInputsCheck.erase(OpName); // It occurred, remove from map.
----------------
Reference rather than copy here? (& probably a reference to the node rather than a reference to the shared_ptr of node)
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp:3550
+ std::vector<TreePatternNodePtr> Children;
Children.push_back(OpNode);
+ OpNode = std::make_shared<TreePatternNode>(Xform, Children,
----------------
maybe std::move here (though it'd mean needing to call getNumTypes above for the expression below)
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp:3555
ResultNodeOperands.push_back(OpNode);
}
----------------
std::move
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp:3562
- TreePatternNode *ResultPattern =
- new TreePatternNode(I->getRecord(), ResultNodeOperands,
- GetNumNodeResults(I->getRecord(), *this));
+ TreePatternNodePtr ResultPattern = std::make_shared<TreePatternNode>(
+ I->getRecord(), ResultNodeOperands,
----------------
Probably 'auto' here.
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp:3986
IterateInference =
- ForceArbitraryInstResultType(Result.getTree(0), Result);
+ ForceArbitraryInstResultType(Result.getTree(0).get(), Result);
} while (IterateInference);
----------------
Maybe another API that could take a node by reference instead of pointer?
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp:4007
// Promote the xform function to be an explicit node if set.
- TreePatternNode *DstPattern = Result.getOnlyTree();
- std::vector<TreePatternNode*> ResultNodeOperands;
+ TreePatternNodePtr DstPattern = Result.getOnlyTree();
+ std::vector<TreePatternNodePtr> ResultNodeOperands;
----------------
Does this need ownership (is Result being changed in such a way that it might cease owning the tree pattern node)? Doesn't look like it.
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp:4079
Preds.insert(Preds.end(), MC.begin(), MC.end());
PatternsToMatch.emplace_back(P.getSrcRecord(), Preds, NewSrc, NewDst,
P.getDstRegs(), P.getAddedComplexity(),
----------------
std::move on NewSrc and NewDst?
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp:4085
for (PatternToMatch &P : Copy) {
- TreePatternNode *SrcP = nullptr, *DstP = nullptr;
+ TreePatternNodePtr SrcP = nullptr, DstP = nullptr;
if (P.SrcPattern->hasProperTypeByHwMode())
----------------
Do these need to be smart pointers? Looks like they don't need ownership between where they're set & where they're passed to collectModes?
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp:4097
if (SrcP)
- collectModes(Modes, SrcP);
+ collectModes(Modes, SrcP.get());
if (DstP)
----------------
Other APIs that could be cleaned up with reference rather than pointer parameters, perhaps.
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp:4209
NewChildren.push_back(ChildVariants[i][Idxs[i]]);
- auto R = llvm::make_unique<TreePatternNode>(
+ TreePatternNodePtr R = std::make_shared<TreePatternNode>(
Orig->getOperator(), NewChildren, Orig->getNumTypes());
----------------
could use "auto" here - since you've named the type on the RHS anyway.
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp:4227
+ none_of(OutVariants, [&](TreePatternNodePtr Variant) {
+ return R->isIsomorphicTo(Variant.get(), DepVars);
}))
----------------
Whenever I'm moving to a smart pointer type I use cases like this as an opportunity to tidy up the code - for example this isIsomorphicTo function could take a `const TreePatternNode&` - then the callers could use op* to dereference - and then the code would be the same between smart and shared pointers (rather than having to move from X to X.get() it'd just be *X in both cases).
I don't remember/can't find right now if we have a dereferencing range adapter - which would be handy to use here if the API was changed as suggested.
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.h:816
+ const TreePatternNodePtr &getTree(unsigned i) const { return Trees[i]; }
+ void setTree(unsigned i, TreePatternNodePtr Tree) { Trees[i] = Tree; }
+ TreePatternNodePtr getOnlyTree() const {
----------------
Missing std::move:
Trees[i] = Tree;
should be:
Trees[i] = std::move(Tree);
================
Comment at: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.h:817
+ void setTree(unsigned i, TreePatternNodePtr Tree) { Trees[i] = Tree; }
+ TreePatternNodePtr getOnlyTree() const {
assert(Trees.size() == 1 && "Doesn't have exactly one pattern!");
----------------
This should probably be const ref - so that users of this that don't need to take ownership don't cause a refcount increment/decrement caused by making the copy.
Repository:
rL LLVM
https://reviews.llvm.org/D47463
More information about the llvm-commits
mailing list