<div class="gmail_quote">On 19 July 2012 14:10, 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="HOEnZb"><div class="h5">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 19 July 2012 12:01, 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 11 July 2012 09:50, 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>
Quoting Nick Lewycky <<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 30 June 2012 13:52, 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">
<br>
Hi Nick,<br>
<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<br>
of<br>
cases where the behavior is different?<br>
<br>
<br>
Maybe. They're accomplishing different things and they test different<br>
</blockquote>
conditions along the way. I'll see what I can do.<br>
<br>
<br>
</blockquote>
The behaviour is now different in every case. :)<br>
</blockquote>
<br>
Updated patch attached!<br>
<br>
</blockquote>
<br>
The patch looks great now! It's interesting to see that it is now able to<br>
delete a store that was not deleted before.<br>
<br>
</blockquote>
<br>
Thanks! It's not deleting a store it previously couldn't, the test change<br>
in 2009-11-16-<u></u>BrokenPerformHeapAllocSRoA.ll shows us deleting the malloc<br>
call, which previously we didn't try to do (but would ultimately be deleted<br>
anyways).<br>
</blockquote>
<br></div></div>
Ah, I see! It's great nevertheless.<div class="im"><br>
<br>
<br>
<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>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- The correctness of the handling of memcpy/memmove/memset depends on the<br>
invariant that the function is only called when the GV is written. I think<br>
we should have a test to make sure we don't break this invariant in the<br>
future (or at least drop an assertion there).<br>
(BTW, I imagine the same invariant is valid in<br>
CleanupConstantGlobalUsers(), so it can use the same simplification you did<br>
here)<br>
<br>
</blockquote>
<br>
I'm not sure what you mean. What is the invariant?<br>
</blockquote>
<br></div>
I meant this:<br>
assert(MSI->getRawDest() == GV);<br>
assert(MTI->getRawDest() == GV);<br>
<br>
The memcpy has to write the GV you're removing; you cannot do a memcpy from the GV to some other variable.<br></blockquote><div><br></div><div>In our case, we know the GV is only stored to.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
CleanupConstantGlobalUsers() checks for that explicitly, but it can probably be turned into an assert().</blockquote><div><br></div><div>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.</div>
<div><br></div><div>Nick</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Also:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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<br>
second round we may not delete anything and return false.<br>
<br>
</blockquote>
<br>
Thanks. Fixed.<br>
<br>
Nick<br>
</blockquote>
</div></div></blockquote></div><br>