<div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 8, 2014 at 8:34 PM, Evan Cheng <span dir="ltr"><<a href="mailto:evan.cheng@apple.com" target="_blank">evan.cheng@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">#2 is doable and I considered it. I rejected it because removing items from worklist just feels wrong to me. Also, visitMemTransferInst did rewrite the memcpy instruction so the logic about revisiting the alloca appears sound. So ultimately I didn't against the approach. If you are confident that's the right approach, I can do it. It would be a very simple fix.</blockquote>
</div><br>I'd like to understand why removing things from the worklist feels wrong first. If it doesn't feel right to both of us, it probably isn't. =]</div><div class="gmail_extra"><br></div><div class="gmail_extra">
The reason it seems like the better approach is this: SROA has successfully rewritten an alloca's uses such that they are promotable. That was the only goal of rewriting the uses. Why should we rewrite them again? The only goal we could have has already been achieved.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">Actually, a separate way of looking at this problem entirely that I like more:</div><div class="gmail_extra"><br></div><div class="gmail_extra">
We don't need to add the alloca we are *currently* rewriting to the worklist. In fact, we only consider the *other* ptr on line 2500. We also know what alloca we're rewriting from (OldAI) and what alloca we're rewriting to (NewAI). Neither of those should *ever* be added to the worklist. If we need to, we'll add NewAI in line 3166 after ensuring it isn't promoted.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">So I think the real bug is that while we look in OtherPtr on line 2500, we don't filter out both OldAI and NewAI from consideration. If we're rewriting both sides of a memcpy, the first time we'll add OldAI, and the second time we'll add NewAI, both of which are wrong. We're papering over this with the code starting in lines 3566. That code handles the OldAI case but not the NewAI case you've hit.</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">I like filtering OldAI and NewAI in visitMemTransferInst way more than the code I have starting at line 3566. I suspect you would have had to add something similar to implement my #2, so lets try the visitMemTransferInst approach?</div>
<div class="gmail_extra"><br></div><div class="gmail_extra">If it works well for your test case, I may also try removing the code at 3566 and adding asserts to take a similar approach elsewhere in SROA.</div></div>