[PATCH] Ensure SROA doesn't prematurely promote an alloca that's being revisited

[Chandler Carruth]

On Wed, Jan 8, 2014 at 8:34 PM, Evan Cheng <evan.cheng at apple.com> wrote:

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


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. =]

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.


Actually, a separate way of looking at this problem entirely that I like
more:

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.

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.

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?

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140108/30c0fad6/attachment.html>