[llvm-commits] preserve pointer roots in globals

[Nuno Lopes]

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.
CleanupConstantGlobalUsers() checks for that explicitly, but it can  
probably be turned into an assert().

Nuno


> 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