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

[Evan Cheng]

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

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

Ideally there should only two operations on a worklist, pushing to the end and popping from the end (or front). If an item is being removed from the worklist, that means it should not have been added in the first place.

> 
> 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?

I'm not quite sure if I understand you completely. Do you mean something simple like this?

Index: SROA.cpp                                                                                                                                                                                                                           
===================================================================
--- SROA.cpp    (revision 198886)                                                                                                                                                                                                         
+++ SROA.cpp    (working copy)                                                                                                                                                                                                            
@@ -2497,8 +2497,12 @@
     // alloca that should be re-examined after rewriting this instruction.                                                                                                                                                               
     Value *OtherPtr = IsDest ? II.getRawSource() : II.getRawDest();                                                                                                                                                                      
     if (AllocaInst *AI                                                                                                                                                                                                                   
-          = dyn_cast<AllocaInst>(OtherPtr->stripInBoundsOffsets()))                                                                                                                                                                      
-      Pass.Worklist.insert(AI);                                                                                                                                                                                                          
+          = dyn_cast<AllocaInst>(OtherPtr->stripInBoundsOffsets())) {                                                                                                                                                                    
+      if (AI != &OldAI && AI != &NewAI)                                                                                                                                                                                                  
+        // Filter out both OldAI (which is being rewritten) and the NewAI                                                                                                                                                                
+        // (which is already being examined).                                                                                                                                                                                            
+        Pass.Worklist.insert(AI);                                                                                                                                                                                                        
+    }

This ended up triggering an assertion later:
Assertion failed: (isAllocaPromotable(AI) && "Cannot promote non-promotable alloca!"), function run, file ../llvm/lib/Transforms/Utils/PromoteMemoryToRegister.cpp, line 535.

I think the issue is the AllocaInst does need to be revisited because it is no longer promotable due to the memcpy being rewritten?

Evan

> 
> 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/20140109/01981f55/attachment.html>