[llvm-commits] preserve pointer roots in globals

[Nick Lewycky]

On 19 July 2012 14:10, Nuno Lopes <nunoplopes at sapo.pt> wrote:

> Quoting Nick Lewycky <nlewycky at google.com>:
>
>  On 19 July 2012 12:01, Nuno Lopes <nunoplopes at sapo.pt> wrote:
>>
>>  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.
>>>
>>>
>> Thanks! It's not deleting a store it previously couldn't, the test change
>> in 2009-11-16-**BrokenPerformHeapAllocSRoA.ll shows us deleting the
>> malloc
>> call, which previously we didn't try to do (but would ultimately be
>> deleted
>> anyways).
>>
>
> Ah, I see! It's great nevertheless.
>
>
>
>  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)
>>>
>>>
>> I'm not sure what you mean. What is the invariant?
>>
>
> I meant this:
> assert(MSI->getRawDest() == GV);
> assert(MTI->getRawDest() == GV);
>
> The memcpy has to write the GV you're removing; you cannot do a memcpy
> from the GV to some other variable.
>

In our case, we know the GV is only stored to.


> CleanupConstantGlobalUsers() checks for that explicitly, but it can
> probably be turned into an assert().


But that assert() would actually fire. :) This matters if you have
something like a memcpy that writes to bitcast(GV). In our case,
CleanupPointerRootUsers doesn't recursively clean up constant expressions,
so we won't be visiting the bitcast and finding the memcpy as a use.
CleanupConstantGlobalUsers does, and it also has to deal with MTI with the
GV as source.

Nick

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. Fixed.
>>
>> Nick
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120719/feabaa68/attachment.html>