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

[Chandler Carruth]

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.


On Thu, Jan 16, 2014 at 11:42 AM, Evan Cheng <evan.cheng at apple.com> wrote:

> Ping again.
>
> Evan
>
> On Jan 13, 2014, at 10:35 AM, Chandler Carruth <chandlerc at google.com>
> wrote:
>
> 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/20140116/92c84ded/attachment.html>