<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Sep 21, 2014 at 8:03 PM, wuhui1973 <span dir="ltr"><<a href="mailto:wuhui1973@163.com" target="_blank">wuhui1973@163.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="line-height:1.7;color:#000000;font-size:14px;font-family:Arial"><div style="line-height:1.7;color:#000000;font-size:14px;font-family:Arial"><div><div><span style="line-height:1.7">Hello David, Andrew:</span><br><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7"><br></div><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7">I spent this weekend in designing the solution using shared_ptr, I have changed tons of code, but always found some object had been released prematurely.</div><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7"><br></div><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7">Seem I had mistakenly made two shared_ptr holding the same object, but I got lost in the huge code change.</div><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7"><br></div><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7">The biggest difference between the self defined "shared_ptr" and std::shared_ptr is: for former the reference counter intrudes into the Tree Node, while for latter</div><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7"><br></div><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7">the reference counter is at different place. So for the self defined "shared_ptr", it is safe to use the pointer to Tree Node directly in most time, except in case of </div><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7"><br></div><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7">pointer self-assignment, for example, in method SimplifyTree. But using std::shared_ptr, it needs take care of all pointer assignments. Seems only using shared_ptr</div><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7"><br></div><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7">in place of all Tree Node pointer is safe, but it will result in huge code change and a not-easy implementation. Especially in some <span style="line-height:1.7">method, "this" pointer will be passed as </span></div><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7"><span style="line-height:1.7"><br></span></div><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7"><span style="line-height:1.7">parameter to other function or even as parameter of recursion, which is difficult to handle and error-prone if using std::shared_ptr.</span></div></div></div></div></div></blockquote><div><br></div><div>FWIW, std::shared_ptr does support intrusive reference counting, if it's a necessary aspect of this scheme (though I prefer to avoid it where possible - it's not always possible or practical, especially when the goal is to fix leaks and not to rewrite all the code). You can do this by inheriting from std::enable_shared_from_this, then calling the inherited "shared_from_this()" member function to acquire a std::shared_ptr when you only have a raw pointer that you know some other code already owns.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="line-height:1.7;color:#000000;font-size:14px;font-family:Arial"><div style="line-height:1.7;color:#000000;font-size:14px;font-family:Arial"><div><div><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7"><span style="line-height:1.7"><br></span></div><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7"><span style="line-height:1.7">That is why I insist on using self-defined "shared_ptr".</span></div><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7"><span style="line-height:1.7"><br></span></div><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7"><span style="line-height:1.7">As for reclaiming cyclic referred objects, I have below explanation:</span></div><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7"><span style="line-height:1.7"><br></span></div><div><div>+ Here we can't just delete the nodes inside the container. Consider this cycle:</div><div>+  //         A          </div><div>+  //       7|  \       if we directly delete node A, then node C will be freed as</div><div>+  //      /     \      no reference now, next B. But at time B returning its memory,</div><div>+  //     /      _\|    it finds the pointer to node A and try to free it as B is the</div><div>+  //    B <------ C    last referrer, that comes the double delete!</div><div>+  //                   So the correct way is to break the reference link, for example,</div><div>+  // set the pointer to A in B to null which is reference counted, then B will free A</div><div>+  // as it is the last referrer. In turn C and B will be freed but not double delete.</div></div></div></div></div></div></blockquote><div><br></div><div>I'm still confused by this & will probably have to find some time to look at the code, but it always boils down to:<br><br>If you haev cycles, reference counting is not the right tool.<br><br>If you are working around the limitations of reference counting because you have cycles by defining a cleanup pass - why not just do all the ownership and destruction there? I'm still not really following, but haven't taken the time to look into the code in more detail.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="line-height:1.7;color:#000000;font-size:14px;font-family:Arial"><div style="line-height:1.7;color:#000000;font-size:14px;font-family:Arial"><div><div><div><div>+  //</div></div><br>I have tidied my code, please have a look.</div><div><br></div><div>Thanks & Regards</div><div><br></div><div>Hui Wu<span class=""><br><br><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7"></div><div style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7"></div><br><span style="line-height:1.7">在 2014-09-19 00:04:00，"David Blaikie" <</span><a href="mailto:dblaikie@gmail.com" style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7" target="_blank">dblaikie@gmail.com</a><span style="line-height:1.7">> 写道：</span><br> </span><blockquote style="color:rgb(0,0,0);font-family:Arial;font-size:14px;line-height:1.7;padding-left:1ex;margin:0px 0px 0px 0.8ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div dir="ltr"><br><div class="gmail_extra"><br><div><div class="h5"><div class="gmail_quote">On Sun, Sep 7, 2014 at 4:48 PM, Andrew Trick <span dir="ltr"><<a href="mailto:atrick@apple.com" target="_blank">atrick@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><span><blockquote type="cite"><div>On Sep 2, 2014, at 8:08 PM, wuhui1973 <<a href="mailto:wuhui1973@163.com" target="_blank">wuhui1973@163.com</a>> wrote:</div><br><div><div style="font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;line-height:1.7;font-size:14px;font-family:Arial"><div>Hi Andrew:<br><br>Please find the anwser on line.<br><br><br></div><div></div><div></div><div><br></div>At 2014-09-03 10:54:21, "Andrew Trick" <<a href="mailto:atrick@apple.com" target="_blank">atrick@apple.com</a>> wrote:<br><blockquote style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><br><blockquote type="cite"><div>On Sep 2, 2014, at 7:22 PM, wuhui1973 <<a href="mailto:wuhui1973@163.com" target="_blank">wuhui1973@163.com</a>> wrote:</div><br><div><div style="font-style:normal;font-variant:normal;font-weight:normal;font-size:14px;line-height:1.7;font-family:Arial;text-transform:none;text-indent:0px;letter-spacing:normal;word-spacing:0px;white-space:normal"><div style="line-height:1.7;font-family:Arial;font-size:14px"><div><br>Hello Andrew, Hal:</div><div> </div><div>I have updated my solution. In these days, I have tried BumpPtrAlloc, but the biggest problem of it is BumpPtrAlloc won't invoke dtor before releasing the memory.</div></div></div></div></blockquote><div><br></div><div>What does the dtor do beside release memory?</div><div> </div><div>[huiwu] it will invoke dtor of its member. For example, in TreePattern object, its dtor will invoke dtor of following members:</div><div> </div><div>Trees (type std::vector<TreePatternNode*>)</div><div>NamedNodes (type StringMap<SmallVector<TreePatternNode*,1> >)</div><div>Args (type std::vector<std::string>)</div><div> </div><div>So if dtor if TreePattern is not invoked, only just memory occupied by this TreePattern instance is freeed, memory allocated on heap by all these containers leak!<br></div></blockquote></div></div></blockquote><div><br></div></span><div>I see how it might be hard to allocate all the referenced data using BumpPtrAllocator. That’s unfortunate, because it would be a good way to design this sort of thing.</div><div><br></div><div>Given that you will continue using normal heap allocation, there is still something I don’t understand about the design. It looks like your adding references to all the nodes to a global container. So if there is one point in the code where no TreePatterns should be alive, such as the point where you would call ReclaimCycle, or destroy the BumpPtrAllocator, why don’t you just explicitly delete all the TreePatterns referenced from your container at that point? There would be no need to reference counting or cycle detection.</div></div></div></blockquote><div><br></div><div>Yeah - I'm confused by the complexity and have the same question.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><br></div><div>-Andy</div><span><div><br></div><br><blockquote type="cite"><div><div style="font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;line-height:1.7;font-size:14px;font-family:Arial"><blockquote style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><blockquote type="cite"><div><div style="font-style:normal;font-variant:normal;font-weight:normal;font-size:14px;line-height:1.7;font-family:Arial;text-transform:none;text-indent:0px;letter-spacing:normal;word-spacing:0px;white-space:normal"><div style="line-height:1.7;font-family:Arial;font-size:14px"><div>And I also consider std::shared_ptr, but not use it for following findings:</div><div> </div><div>1> Nodes of TreePatternNode & TreePattern form cycles in reference, I have designed a simple method to recycle them, which can't be used with shared_ptr.</div><div>2> After adding the grammar sugar operator-> & operator T(), now most source files using TreePatternHolder & TreePatternNodeHolder now needn't be changed. We can do that because we check reference count on every reference counted objects in every related methods. So any abuse can be found earily as possible.</div></div></div></div></blockquote><div><br></div><div>Thanks.</div><div><br><blockquote type="cite"><div style="font-style:normal;font-variant:normal;font-weight:normal;font-size:14px;line-height:1.7;font-family:Arial;text-transform:none;text-indent:0px;letter-spacing:normal;word-spacing:0px;white-space:normal"><div style="line-height:1.7;font-family:Arial;font-size:14px"><div> <span style="line-height:1.7">In attachment, there are two kinds of suffix:</span></div><div> </div><div>*.review is for review</div><div>*.fix is for submit</div></div></div></blockquote><div><br></div><div>You can run clang-format before your patch, submit that as a separate patch (no need for a separate review), then submit your patch on top of it.</div><div><br></div><div>-Andy</div><br><blockquote type="cite"><div style="font-style:normal;font-variant:normal;font-weight:normal;font-size:14px;line-height:1.7;font-family:Arial;text-transform:none;text-indent:0px;letter-spacing:normal;word-spacing:0px;white-space:normal"><div style="line-height:1.7;font-family:Arial;font-size:14px"><div><span style="line-height:1.7"> </span></div></div></div></blockquote><blockquote type="cite"><div style="font-style:normal;font-variant:normal;font-weight:normal;font-size:14px;line-height:1.7;font-family:Arial;text-transform:none;text-indent:0px;letter-spacing:normal;word-spacing:0px;white-space:normal"><div style="line-height:1.7;font-family:Arial;font-size:14px"><div>The reason why need these files is because I had run clang-format on the related source files which results in too many unrelated differences<br><br><br></div><div></div><div></div><div><br></div>At 2014-08-07 11:59:44, "Andrew Trick" <<a href="mailto:atrick@apple.com" target="_blank">atrick@apple.com</a>> wrote:<br><blockquote style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><br><div><blockquote type="cite"><div>On Aug 7, 2014, at 2:29 AM, wuhui1973 <<a href="mailto:wuhui1973@163.com" target="_blank">wuhui1973@163.com</a>> wrote:</div><br><div><span style="font-style:normal;font-variant:normal;font-weight:normal;font-size:14px;line-height:23px;font-family:Arial;text-transform:none;text-indent:0px;letter-spacing:normal;word-spacing:0px;float:none;white-space:normal;display:inline!important">>> [hui] I am new to tablgen, I hope I can explain it clearly. At some stage in building llvm, tablgen will compile .td file into .inc file (C++ source), it will compile .td file one after one till the stage is done. As now the tree nodes are just allocated but not freed, they get lost </span><span style="font-style:normal;font-variant:normal;font-weight:normal;font-size:14px;line-height:1.7;font-family:Arial;text-transform:none;text-indent:0px;letter-spacing:normal;word-spacing:0px;white-space:normal">between handling .td files</span><span style="font-style:normal;font-variant:normal;font-weight:normal;font-size:14px;line-height:1.7;font-family:Arial;text-transform:none;text-indent:0px;letter-spacing:normal;word-spacing:0px;white-space:normal">. So the patch aims to reclaim the memory when finishing one .td file. I think pool allocator can be an option, but as I know, pool allocator is just another sub-project under llvm, has it been built into llvm? And it needs be done by the DSA (data structure analysis) alogrithm?! Is there any example in llvm?</span></div></blockquote></div><br><div>Sorry, I wasn’t clear what I was talking about. In LLVM, we have a class called BumpPtrAllocator. You can use that to allocate a set of nodes, then free them all between passes. If that accomplishes our goals for TableGen, then I think it’s the ideal solution.</div><div><br></div><div>-Andy</div></blockquote></div></div><br style="font-style:normal;font-variant:normal;font-weight:normal;font-size:12px;line-height:normal;font-family:Helvetica;text-transform:none;text-indent:0px;letter-spacing:normal;word-spacing:0px;white-space:normal"><br style="font-style:normal;font-variant:normal;font-weight:normal;font-size:12px;line-height:normal;font-family:Helvetica;text-transform:none;text-indent:0px;letter-spacing:normal;word-spacing:0px;white-space:normal"><span title="neteasefooter" style="font-style:normal;font-variant:normal;font-weight:normal;font-size:12px;line-height:normal;font-family:Helvetica;text-transform:none;text-indent:0px;letter-spacing:normal;word-spacing:0px;white-space:normal"><span></span></span><span><DAGISelMatcherGen.cpp.review></span><span><TableGen.cpp.fix></span><span><DAGISelMatcherGen.cpp.fix></span><span><CodeGenDAGPatterns.cpp.fix></span><span><CodeGenDAGPatterns.h.fix></span><span><CodeGenDAGPatterns.h.review></span><span><CodeGenDAGPatterns.cpp.review></span></blockquote></div></blockquote></div></div></blockquote></span></div><br></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div></div></div></div>
</blockquote></div></div></div></div><br><br><span title="neteasefooter"><span></span></span></blockquote></div><br></div></div>