Solution for memory leak in tblgen

[Andrew Trick]

> On Jul 28, 2014, at 1:53 AM, wuhui1973 <wuhui1973 at 163.com> wrote:
> 
> Hello Hal, Andy, Nadav, Tom & all:
> 
> I have worked out a solution for the memory leak problem in tblgen. This memory leak problem is known and deliberatly left by the author for later fix.
> 
> My solution follows the author's suggestion - using reference counter to track the TreePatternNode & TreePattern objects.
> 
> Besides, though TreePatternNodes form DAGs, but with TreePattern, they can form cyclic graph, the solution also includes
> 
> a method to break the cyclic reference. The solution detail please refer to the attachment.
> 
> I have checked the solution with following actions:
> 
> 1> Check tblgen running log, it looks like this:
> 
>      llvm[3]: Building X86.td register info implementation with tblgen
>      llvm[3]: Building X86.td instruction information with tblgen
>     1. NumTPNAlloc: 84347, NumTPNRel: 20190            <--- TPN: TreePatternNode
>     1. NumTPAlloc: 13441, NumTPRel: 9215                  <--- TP: TreePattern
>     2. NumTPNAlloc: 84347, NumTPNRel: 83879
>     2. NumTPAlloc: 13441, NumTPRel: 13271
>     468 TPNs remain, 170 TPs remain                            <--- These are cyclic referred nodes
>     Release done
>     3. NumTPNAlloc: 84347, NumTPNRel: 84347
>     3. NumTPAlloc: 13441, NumTPRel: 13441
> 
>     Has confirmed all allocated objects get released during the loadbuild for all supported targets.
> 
> 2> Compare the generated .inc file with those generated by sainty code. Has confirmed they all the same.
> 
> 3> run "make check" to verify the generated llvm-programs, gets following result:
> It contains one error, but the sainty code also gives the same result! I will raise the bug report later.
> FAIL: LLVM :: ExecutionEngine/RuntimeDyld/AArch64/MachO_ARM64_relocations.s (6206 of 11281)
> ******************** TEST 'LLVM :: ExecutionEngine/RuntimeDyld/AArch64/MachO_ARM64_relocations.s' FAILED ********************
> Script:
> --
> /sources/new/build_llvm/Debug+Asserts/bin/llvm-mc -triple=arm64-apple-ios7.0.0 -code-model=small -relocation-model=pic -filetype=obj -o /sources/new/build_llvm/test/ExecutionEngine/RuntimeDyld/AArch64/Output/foo.o /sources/new/llvm/test/ExecutionEngine/RuntimeDyld/AArch64/MachO_ARM64_relocations.s
> sed "s,<filename>,/sources/new/build_llvm/test/ExecutionEngine/RuntimeDyld/AArch64/Output/foo.o,g" /sources/new/llvm/test/ExecutionEngine/RuntimeDyld/AArch64/MachO_ARM64_relocations.s > /sources/new/build_llvm/test/ExecutionEngine/RuntimeDyld/AArch64/Output/foo.s
> /sources/new/build_llvm/Debug+Asserts/bin/llvm-rtdyld -triple=arm64-apple-ios7.0.0 -verify -check=/sources/new/build_llvm/test/ExecutionEngine/RuntimeDyld/AArch64/Output/foo.s /sources/new/build_llvm/test/ExecutionEngine/RuntimeDyld/AArch64/Output/foo.o
> --
> Exit Code: 1
> 
> Command Output (stderr):
> --
> Expression 'decode_operand(adrp2, 1) = (stub_addr(/sources/new/build_llvm/test/ExecutionEngine/RuntimeDyld/AArch64/Output/foo.o, __text, _ptr)[32:12] - adrp2[32:12])' is false: 0xfff00000 != 0x0
> /sources/new/build_llvm/Debug+Asserts/bin/llvm-rtdyld: error: some checks in '/sources/new/build_llvm/test/ExecutionEngine/RuntimeDyld/AArch64/Output/foo.s' failed
> --
> ********************
> Testing Time: 3155.15s
> ********************
> Failing Tests (1):
>     LLVM :: ExecutionEngine/RuntimeDyld/AArch64/MachO_ARM64_relocations.s
> 
>   Expected Passes    : 11144
>   Expected Failures  : 101
>   Unsupported Tests  : 35
>   Unexpected Failures: 1
> 
> So I think my solution is correct. Could you review it? Or help to forward to someone concerned? Thanks!

Thanks for working on this. I've never looked at this code, so I'd like someone else to review it too. But here are my comments.

At a high-level this looks like a very large diff to accomplish the task at hand.

First, what are we accomplishing? If the goal is to ensure all memory is freed between "passes", then that can be much better done with a pool allocator. Is there really any need to eagerly free tree nodes before the end of a pass?

Another reviewer commented that it looks just like a reimplementation of std::shared_ptr, and I did not completely follow your response. I *think* the only salient difference is that your reference counts are intrusive--they are members of the tree nodes. This allows you to check the RefCnt on any method call. The down side is that you're adding a lot of boilerplate code in the form of asserts and calls to "get()". I won't object to the patch purely for this reason, but you should know that smaller, more self-contained patches are much more likely to be accepted.

I don't understand why we need derived classes TreePatternNodeHolder and TreePatternHolder. The code looks completely generic to me.

This

+  if (!TPContainer.empty())
+  {
+      do

should be 

+  while (!TPContainer.empty())

Why doesn't the copy ctor increment?

+ TreePatternNodeHolder(const TreePatternNodeHolder &rhs):
+      NodeType(rhs.Member) {}

Shouldn't you have a move ctor that does not increment?

Why are the creation routines members of TreePatternNodeHolder rather than returning a new holder?

+TreePatternNode* TreePatternNodeHolder::createTreePatternNode(Init *val, unsigned NumResults)

What is the purpose of changes like this:

-  if (N->getChild(0)->isLeaf() || N->getChild(0)->getOperator() != Operator)
-    Children.push_back(N->getChild(0));
+  TreePatternNode *child0 = N->getChild(0);
+  TreePatternNode *child1 = N->getChild(1);
+
+  if (child0->isLeaf() || child0->getOperator() != Operator)
+    Children.push_back(child0);

I assume you enabled build debug output to your patch for demonstration purposes. Obviously we can't really do that.

Before it gets checked in, your should run clang-format on your code.

typo: remmeber

I wasn't able to review every line. You'll need to find another reviewer.

One suggestion to help review: divide the patch into at least two patch sets. One that introduces your new classes and algorithm. A second patch set that renames a bunch of types in the existing code, and adds "getters" if really necessary.

-Andy

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140806/9ef8cbac/attachment.html>