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

[Chandler Carruth]

On Thu, Jan 16, 2014 at 11:45 AM, Chandler Carruth <chandlerc at google.com>wrote:

> I've looked at this, and am continuing to look at it.
>
> I think your trivial patch is correct, and I don't (yet) understand what
> isn't working. This may be a factor of me needing to be hit with a clue
> bat, and so I've not stopped looking to understand the underlying issue.
>

Jeeze. I'm glad I kept looking. I should have seen this sooner, but totally
missed it.

So, the idea of not adding the old or new alloca to the worklist is kind-of
on the right track. But the story is more convoluted than just that.

When you filter out adding these to the worklist, some things start to work
correctly, but you go on to hit an assertion about being unable to promote
things. This is actually a huge clue, little did I realize. So why does
processing the alloca one more time work when keeping it out of the
worklist doesn't work? We don't actually change *anything* when we
re-process the alloca!

Well, something *else* happens: we garbage collect the to-be-deleted
instructions and actually delete them. It is one of those that the
assertion is complaining about. This deleted instruction happens to be the
memcpy itself.

Looking at the test case and how the memcpy gets onto the list to be
deleted fills in the rest of the clues. What's happening here is that one
side of the memcpy is outside the bound of the alloca entirely and the
other side is just fine. We mark the entire memcpy as dead (which is
correct!) but don't do anything to update the other half which gets onto
the rewrite queue nor do we do anything to remove the to-be-deleted
out-of-bounds use of the alloca. So we had a deleted memcpy (and its
associated GEPs) still pointing back at the alloca when we tried to promote
it. By taking another trip around the alloca worklist, we end up actually
deleting that memcpy by chance and things "work".

Interestingly, the deal with filtering out old and new allocas as your
small patch does is redundant. The very fact that we reached the old alloca
was itself a symptom of the bug! That code path is below a comment that
says the alloca is only used by one side of the memcpy, not both sides. We
handle memcpy between two regions of an alloca very specially before
reaching that point.

So, I've added an assert rather than a test, and I've fixed the code to
properly clean up all the loose ends when we find an out-of-bounds memcpy
argument. We have to kill it, kill any already added slice, and check for
dead instructions before adding a new slice. This allows your test case
(somewhat reduced and condensed) to pass with flying colors. Check out
r199590.

Thanks for bringing all this up, and sorry it didn't prove easy to find the
root cause. I'm just *really* glad to have had the nice test case to go
chase down this nastiness.
-Chandler
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140119/d3feb21c/attachment.html>