This is an archive of the discontinued LLVM Phabricator instance.

[TableGen] Avoid leaking TreePatternNodes by using shared_ptr.
ClosedPublic

Authored by fhahn on May 29 2018, 2:40 AM.

Details

Summary

By using std::shared_ptr for TreePatternNode, we can avoid leaking them.
Together with the other patches in linked to this one, ASAN shipped with
GCC8 is happy for TableGen.

I tried to keep the number of times we copy shared_ptrs around small,
but I probably missed a few places where there is no need to copy them.

Diff Detail

Event Timeline

Generally LGTM. I have a couple questions on this one though

utils/TableGen/CodeGenDAGPatterns.h
923

Given that we build with -fno-exceptions, is the noexcept really needed?

932–935

Does this object really own Pattern? It looks like it probably does but it's a raw pointer. If it really does own it we should be using unique_ptr

fhahn updated this revision to Diff 148941.May 29 2018, 10:35 AM

Thanks Daniel! I removed the changes related to leaking DAGInstruction::Pattern. I will add a separate patch for that, it should be easier to review that way.

fhahn added inline comments.May 29 2018, 11:32 AM
utils/TableGen/CodeGenDAGPatterns.h
932–935

Ah that change is unrelated to the shared_ptr changes. I will try to move it out, it should be clearer then.

fhahn marked an inline comment as done.May 29 2018, 11:36 AM
This revision is now accepted and ready to land.May 30 2018, 8:11 AM
This revision was automatically updated to reflect the committed changes.

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.

llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp
2522 ↗(On Diff #149197)

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)

2674–2676 ↗(On Diff #149197)

auto & std::move? Or maybe just inline it all together:

Children.insert(Children.begin(), std::make_shared<TreePatternNode>(IntInit::get(IID), 1));
2681 ↗(On Diff #149197)

Node reference (eg: "auto &Child = *Children[i];") maybe? Doesn't look like this needs ownership?

2700 ↗(On Diff #149197)

auto, maybe

2737 ↗(On Diff #149197)

std::move?

3096 ↗(On Diff #149197)

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?

3448 ↗(On Diff #149197)

Node reference here, doesn't look like this needs any ownership?

3488 ↗(On Diff #149197)

std::move? (after moving this down past the one other use of RNode)

3529 ↗(On Diff #149197)

Reference rather than copy here? (& probably a reference to the node rather than a reference to the shared_ptr of node)

3550 ↗(On Diff #149197)

maybe std::move here (though it'd mean needing to call getNumTypes above for the expression below)

3555 ↗(On Diff #149197)

std::move

3562 ↗(On Diff #149197)

Probably 'auto' here.

3986 ↗(On Diff #149197)

Maybe another API that could take a node by reference instead of pointer?

4007 ↗(On Diff #149197)

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.

4079 ↗(On Diff #149197)

std::move on NewSrc and NewDst?

4085 ↗(On Diff #149197)

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?

4097 ↗(On Diff #149197)

Other APIs that could be cleaned up with reference rather than pointer parameters, perhaps.

4209 ↗(On Diff #149197)

could use "auto" here - since you've named the type on the RHS anyway.

4227 ↗(On Diff #149197)

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.

llvm/trunk/utils/TableGen/CodeGenDAGPatterns.h
816 ↗(On Diff #149197)

Missing std::move:

Trees[i] = Tree;

should be:

Trees[i] = std::move(Tree);
817 ↗(On Diff #149197)

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.

fhahn added a comment.Jun 4 2018, 1:58 PM

Thanks you very much for looking at this carefully! I thought it was safer to start with a smaller diff, but that it's in now and I'll prepare a patch with your suggestions.