[llvm-commits] preserve pointer roots in globals

[Duncan Sands]

Hi Nick, if the global has private (as opposed to internal) linkage then it
should always be safe to remove the stores I guess.

Ciao, Duncan.

On 19/07/12 08:30, Nick Lewycky wrote:
> On 11 July 2012 09:50, Nuno Lopes <nunoplopes at sapo.pt
> <mailto:nunoplopes at sapo.pt>> wrote:
>
>     Quoting Nick Lewycky <nlewycky at google.com <mailto:nlewycky at google.com>>:
>
>         On 30 June 2012 13:52, Nuno Lopes <nunoplopes at sapo.pt
>         <mailto: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
>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>