[llvm-commits] preserve pointer roots in globals

[Nick Lewycky]

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

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?

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/f689b943/attachment.html>