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

[Evan Cheng]

On Jan 8, 2014, at 1:49 PM, Chandler Carruth <chandlerc at google.com> wrote:

> 
> On Wed, Jan 8, 2014 at 3:10 PM, Evan Cheng <evan.cheng at apple.com> wrote:
> Attached is a patch (and the test case) that fix a SROA edge case. An alloca can be added back to the worklist to be revisited during rewrite of a MemTransferInst. If the alloca is added to the "promotable" queue and then added again when it's revisited, mem2reg will crash. The patch simply prevents promotion of an alloca that is scheduled to be revisited.
> 
> Ow! Good catch!
>  
> 
> The patch isn't ideal. It's following the AllocaSliceRewriter::isUsedByRewrittenSpeculatableInstructions() example by caching a boolean state. But it's correct and as clean a patch as I can come up with.
> 
> Hmm, yea. I hated isUsedByRewrittenSpeculatableInstructions when it went in, but I still don't have a better solution. However, there might be a better solution here. The speculation case is particularly hard because the place where this is being "used" can't easily be updated. But this case might be easier. Could you instead of this, either:
> 
> 1) Check for promotability prior to adding the alloca back to the worklist (I doubt this would work), or
> 2) Remove the alloca from the worklist (or skip it when popping it off of the worklist) when it has already be slated for promotion?
> 
> I'm hopeful that we could do #2, but not confident. I feel like we really should go ahead and promote in this case rather than carrying on and re-processing the alloca *again* when it is already in a good state.
> 

#1 doesn't work given the way the code is currently structured. Promotability is only decided after all of the rewritings are done. So visitMemTransferInst would not have this information.

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

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