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

[Chandler Carruth]

Sorry, fell off my radar. I'll look at this today.


On Mon, Jan 13, 2014 at 10:31 AM, Evan Cheng <evan.cheng at apple.com> wrote:

> Chandler, ping?
>
> Evan
>
> On Jan 9, 2014, at 4:29 PM, Evan Cheng <evan.cheng at apple.com> wrote:
>
>
> 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.
>
>
> _______________________________________________
> 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/20140113/35b74e42/attachment.html>