<div class="gmail_quote">On 11 July 2012 09:50, 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">

<div class="HOEnZb"><div class="h5">Quoting Nick Lewycky <<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>>:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 30 June 2012 13:52, Nuno Lopes <<a href="mailto:nunoplopes@sapo.pt" target="_blank">nunoplopes@sapo.pt</a>> wrote:<br>
<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<br>
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()<br>
and CleanupConstantGlobalUsers(). Can't you just single out the couple of<br>
cases where the behavior is different?<br>
<br>
</blockquote>
<br>
Maybe. They're accomplishing different things and they test different<br>
conditions along the way. I'll see what I can do.<br></blockquote></div></div></blockquote><div><br></div><div>The behaviour is now different in every case. :)</div><div><br></div><div>Updated patch attached!</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"><div class="HOEnZb"><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<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.<br>
I believe the patch should learn a few tricks to remove malloc calls before<br>
going in. Otherwise I fear that no one will revisit this issue for a long<br>
time..  If the pointer doesn't escape anywhere else besides the store to<br>
the global, then it can be safely removed. That's hopefully easy to<br>
implement.<br>
<br>
</blockquote>
<br>
I've thought about this for a while and I have a new crazy idea. What if I<br>
move all this dead-malloc work to a new malloc optimization pass. We need<br>
one of these anyways. We should have a pass that turns unescaping small<br>
mallocs into allocas, and if the pointer is captured by a global we can<br>
check whether the global is never-loaded and decide to delete it after all.<br>
Yes globalopt already knows that the global is never loaded, but that's<br>
relatively easy to calculate and I don't want to have two passes (globalopt<br>
and heap-stack-promotion) doing relatively expensive capture tracking.<br>
<br>
Does this make sense? Or have I gone off the deep end?<br>
</blockquote>
<br>
<br></div></div>
Ok, so we have two separate issues: deleting unused mallocs and promoting them to allocas.<br>
<br>
Right now instcombine knows how to delete malloc which are never loaded  (while GVN will delete mallocs that are never stored before being loaded). I think it's fairly easy to extend instcombine to also delete mallocs that have their result stored in a global but not loaded. I can implement that.<br>


<br>
About the stack promotion, I thought LLVM was already doing that, but I've just confirmed that it's not.<br>
Can we teach, say Scalar Repl, to do that?  I'm just trying to avoid adding a new pass for a relatively small optimization (in terms of code, at least).<br>
<br>
Anyway, I would say that you should commit your patch to GlobalOpt, and I can make instcombine smarter sometime later this week.  And we should discuss where to put the heap2stack thing..<span class="HOEnZb"><font color="#888888"><br>


<br>
Nuno</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><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<br>
the behavior being introduced, to make sure no one accidently reverts the<br>
patch in a few years.<br>
<br>
</blockquote>
<br>
I'm hoping that using names like "isLeakCheckerRoot()" will make it clear<br>
what's going on.<br>
<br>
Nick<br>
<br>
<br>
<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<br>
<br>
<br>
The attached patch improves the interaction between GlobalOpt and leak<br>
checkers. One of the things that GlobalOpt will do is delete all the stores<br>
to a global that isn't loaded. If for some reason we don't mop up the<br>
malloc/new which was being stored in that global, a leak checker will<br>
complain.<br>
<br>
The patch handles this by not deleting stores to globals in the above<br>
case, when the global is known to be a pointer type, or a composite type<br>
that contains one. We do delete stores of Constant, since those are<br>
trivially not derived from calling an allocator. There's more analysis we<br>
could do here to win back optimization if it turns out to be really<br>
important.<br>
<br>
Please review!<br>
<br>
Nick<br>
</blockquote></blockquote>
</div></div></blockquote></div><br>