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

Evan Cheng evan.cheng at apple.com
Mon Jan 20 09:41:21 PST 2014


On Jan 19, 2014, at 10:54 AM, Evan Cheng <evan.cheng at apple.com> wrote:

> Thanks. What's the revision number of your patch? I don't see r199590.

Nevermind. I see it now.

Thanks,

Evan

> 
> Evan
> 
> On Jan 19, 2014, at 4:26 AM, Chandler Carruth <chandlerc at google.com> wrote:
> 
>> 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
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140120/21986167/attachment.html>


More information about the llvm-commits mailing list