[llvm-commits] preserve pointer roots in globals

Nuno Lopes nunoplopes at sapo.pt
Wed Jul 11 09:50:32 PDT 2012


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.
>
>
>> - 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



More information about the llvm-commits mailing list