<div class="gmail_quote">On 19 July 2012 12:01, Nuno Lopes <span dir="ltr"><<a href="mailto:nunoplopes@sapo.pt" target="_blank">nunoplopes@sapo.pt</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">Quoting Nick Lewycky <<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>>:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 11 July 2012 09:50, Nuno Lopes <<a href="mailto:nunoplopes@sapo.pt" target="_blank">nunoplopes@sapo.pt</a>> wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Quoting Nick Lewycky <<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>>:<br>
<br>
On 30 June 2012 13:52, Nuno Lopes <<a href="mailto:nunoplopes@sapo.pt" target="_blank">nunoplopes@sapo.pt</a>> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Hi Nick,<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I understand the use case that you and Chandler presented in the other<br>
emails, and I believe it's a perfectly valid one!<br>
However, I've 2 concerns about this patch:<br>
- it seems there's some code duplication between<br>
CleanupPointerRootUsers()<br>
and CleanupConstantGlobalUsers(). Can't you just single out the couple of<br>
cases where the behavior is different?<br>
<br>
<br>
</blockquote>
Maybe. They're accomplishing different things and they test different<br>
conditions along the way. I'll see what I can do.<br>
<br>
</blockquote>
<br>
</blockquote>
The behaviour is now different in every case. :)<br>
<br>
Updated patch attached!<br>
</blockquote>
<br></div>
The patch looks great now! It's interesting to see that it is now able to delete a store that was not deleted before.<br></blockquote><div><br></div><div>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).</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Just a few minor comments about CleanupPointerRootUsers():<br>
- 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).<br>
(BTW, I imagine the same invariant is valid in CleanupConstantGlobalUsers(), so it can use the same simplification you did here)<br></blockquote><div><br></div><div>I'm not sure what you mean. What is the invariant?</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Also:<br>
<br>
+ } else if (Constant *C = dyn_cast<Constant>(U)) {<br>
+ if (SafeToDestroyConstant(C)) {<br>
+ C->destroyConstant();<br>
+ // This could have invalidated UI, start over from scratch.<br>
+ Dead.clear();<br>
+ return CleanupPointerRootUsers(GV);<br>
+ }<br>
<br>
Here you have to 'return true', there's no escape possible because in the second round we may not delete anything and return false.<br></blockquote><div><br></div><div>Thanks. Fixed.</div><div><br></div><div>
Nick</div><div><br></div></div>