[llvm-commits] preserve pointer roots in globals

[Nick Lewycky]

On 11 July 2012 09:50, Nuno Lopes <nunoplopes at sapo.pt> wrote:

> Quoting Nick Lewycky <nlewycky at google.com>:
>
>  On 30 June 2012 13:52, Nuno Lopes <nunoplopes at sapo.pt> wrote:
>>
>>  Hi Nick,
>>>
>>> I understand the use case that you and Chandler presented in the other
>>> emails, and I believe it's a perfectly valid one!
>>> However, I've 2 concerns about this patch:
>>> - 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?
>>>
>>>
>> Maybe. They're accomplishing different things and they test different
>> conditions along the way. I'll see what I can do.
>>
>
The behaviour is now different in every case. :)

Updated patch attached!

Nick


> - 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.
>>>
>>>
>> 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.
>>
>> Does this make sense? Or have I gone off the deep end?
>>
>
>
> Ok, so we have two separate issues: deleting unused mallocs and promoting
> them to allocas.
>
> 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.
>
> About the stack promotion, I thought LLVM was already doing that, but I've
> just confirmed that it's not.
> 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).
>
> 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..
>
> Nuno
>
>
>
>  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.
>>>
>>>
>> I'm hoping that using names like "isLeakCheckerRoot()" will make it clear
>> what's going on.
>>
>> Nick
>>
>>
>>
>>> Nuno
>>>
>>>
>>> -----Original Message----- From: Nick Lewycky
>>> Sent: Friday, June 29, 2012 4:08 AM
>>> To: Commit Messages and Patches for LLVM
>>> Subject: [llvm-commits] preserve pointer roots in globals
>>>
>>>
>>> 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.
>>>
>>> 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.
>>>
>>> Please review!
>>>
>>> Nick
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120718/ef429781/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: globalopt-ptrroots-2.patch
Type: application/octet-stream
Size: 9063 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120718/ef429781/attachment.obj>