<div class="gmail_quote">On 30 June 2012 13:52, Nuno Lopes <span dir="ltr"><<a href="mailto:nunoplopes@sapo.pt" target="_blank">nunoplopes@sapo.pt</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

Hi Nick,<br>
<br>
I understand the use case that you and Chandler presented in the other emails, and I believe it's a perfectly valid one!<br>
However, I've 2 concerns about this patch:<br>
- it seems there's some code duplication between CleanupPointerRootUsers() and CleanupConstantGlobalUsers(). Can't you just single out the couple of cases where the behavior is different?<br></blockquote><div><br>

</div><div>Maybe. They're accomplishing different things and they test different conditions along the way. I'll see what I can do.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


- I'm not totally confortable with disabling such class of optimizations. I believe the patch should learn a few tricks to remove malloc calls before going in. Otherwise I fear that no one will revisit this issue for a long time..  If the pointer doesn't escape anywhere else besides the store to the global, then it can be safely removed. That's hopefully easy to implement.<br>

</blockquote><div><br></div><div>I've thought about this for a while and I have a new crazy idea. What if I move all this dead-malloc work to a new malloc optimization pass. We need one of these anyways. We should have a pass that turns unescaping small mallocs into allocas, and if the pointer is captured by a global we can check whether the global is never-loaded and decide to delete it after all. Yes globalopt already knows that the global is never loaded, but that's relatively easy to calculate and I don't want to have two passes (globalopt and heap-stack-promotion) doing relatively expensive capture tracking.</div>

<div><br></div><div>Does this make sense? Or have I gone off the deep end?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">BTW, I would add a few more comments to the code explaining the reason for the behavior being introduced, to make sure no one accidently reverts the patch in a few years.<br>

</blockquote><div><br></div><div>I'm hoping that using names like "isLeakCheckerRoot()" will make it clear what's going on.</div><div><br></div><div>Nick</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">


<br>
Nuno<br>
<br>
<br>
-----Original Message----- From: Nick Lewycky<br>
Sent: Friday, June 29, 2012 4:08 AM<br>
To: Commit Messages and Patches for LLVM<br>
Subject: [llvm-commits] preserve pointer roots in globals<div class="HOEnZb"><div class="h5"><br>
<br>
The attached patch improves the interaction between GlobalOpt and leak checkers. One of the things that GlobalOpt will do is delete all the stores to a global that isn't loaded. If for some reason we don't mop up the malloc/new which was being stored in that global, a leak checker will complain.<br>


<br>
The patch handles this by not deleting stores to globals in the above case, when the global is known to be a pointer type, or a composite type that contains one. We do delete stores of Constant, since those are trivially not derived from calling an allocator. There's more analysis we could do here to win back optimization if it turns out to be really important.<br>


<br>
Please review!<br>
<br>
Nick <br>
</div></div></blockquote></div><br>