[llvm-commits] preserve pointer roots in globals

[Nuno Lopes]

Quoting Nick Lewycky <nlewycky at google.com>:

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

The patch looks great now!  It's interesting to see that it is now  
able to delete a store that was not deleted before.

Just a few minor comments about CleanupPointerRootUsers():
  - The correctness of the handling of memcpy/memmove/memset depends  
on the invariant that the function is only called when the GV is  
written. I think we should have a test to make sure we don't break  
this invariant in the future  (or at least drop an assertion there).
(BTW, I imagine the same invariant is valid in  
CleanupConstantGlobalUsers(), so it can use the same simplification  
you did here)


Also:

+    } else if (Constant *C = dyn_cast<Constant>(U)) {
+      if (SafeToDestroyConstant(C)) {
+        C->destroyConstant();
+        // This could have invalidated UI, start over from scratch.
+        Dead.clear();
+        return CleanupPointerRootUsers(GV);
+      }

Here you have to 'return true', there's no escape possible because in  
the second round we may not delete anything and return false.

Thanks,
Nuno


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