[PATCH] Allow blocks to be merged when one has an undef input to a phi.

[Mark Lacey]

Ping?

Can someone please review the attached patch and commit if it looks good?

I've simplified things a bit more and regenerated the patch against TOT.

Thanks,

Mark


TryToSimplifyUncondBranchFromEmptyBlock was checking that any common
predecessors of the two blocks it is attempting to merge supply the
same incoming values to any phi in the successor block. This change
allows merging in the case where there is one or more incoming values
that are undef. The undef values are rewritten to match the non-undef
value that flows from the other edge.

Merged the tests from
- test/Transforms/SimplifyCFG/2008-05-16-PHIBlockMerge.ll
into
- test/Transforms/SimplifyCFG/EqualPHIEdgeBlockMerge.ll
and also added tests to this file for the situation addressed by this
patch.

<rdar://problem/12801861>
---
lib/Transforms/Utils/Local.cpp                     |  76 ++++++-
.../SimplifyCFG/2008-05-16-PHIBlockMerge.ll        | 131 -----------
.../SimplifyCFG/EqualPHIEdgeBlockMerge.ll          | 240 ++++++++++++++++++++-
3 files changed, 307 insertions(+), 140 deletions(-)
delete mode 100644 test/Transforms/SimplifyCFG/2008-05-16-PHIBlockMerge.ll





On Jun 8, 2013, at 1:38 PM, Mark Lacey <mark.lacey at apple.com> wrote:

> Hi Duncan,
> 
> I just realized I re-attached the old patch when I sent this. I've now attached the correct patch.
> 
> Thanks,
> 
> Mark
> 
> <0001-Allow-blocks-to-be-merged-when-one-has-an-undef-inpu.patch>
> 
> On Jun 6, 2013, at 7:15 PM, Mark Lacey <mark.lacey at apple.com> wrote:
> 
>> Hi Duncan,
>> 
>> On Jun 6, 2013, at 11:26 AM, Duncan Sands <duncan.sands at gmail.com> wrote:
>>> Hi Mark,
>>> 
>>> On 06/06/13 19:27, Mark Lacey wrote:
>>>> Hi Duncan,
>>>> 
>>>> On Jun 6, 2013, at 7:21 AM, Duncan Sands <duncan.sands at gmail.com> wrote:
>>>>>> All of the predecessors of BB must be added to the phi node since we rewrite all of the uses of BB (in terminators) to target Succ. Otherwise we would have a conditional branch or switch that branches to Succ on multiple edges (e.g. both sides of the conditional branch, or multiple cases of the switch), but not have as many incoming edges in the phi.
>>>>> 
>>>>> if (PredBBIdx >= 0) was true, then PN already has PredBB has a predecessor.
>>>>> Indeed, you just assigned a value to it in the line
>>>>> PN->setIncomingValue(PredBBIdx, PredVal);
>>>>> But then you fall through to the line
>>>>> PN->addIncoming(PredVal, PredBB);
>>>>> and add a second copy of PredBB and PredVal to PN.  Isn't this wrong?
>>>> 
>>>> We need to add the incoming values to the phi for all the new edges being redirected from the predecessor (this was already happening before my change as well).
>>> 
>>> thanks for the detailed explanation.  However I don't understand how you handle
>>> the case where PrevBB is already present multiple times in PN.  I think you
>>> only update the first instance of PrevBB, not all instances.
>>> 
>> 
>> Yes, you're quite right. In the case where the block being merged into had a phi with multiple undef inputs from the common predecessor, only some of them will be updated.
>> 
>> I have attached a new patch that addresses this, and also restructures my changes to make them a bit more straightforward and clear.
>> 
>> Thanks again for your time and feedback,
>> 
>> Mark
>> 
>> <0001-Allow-blocks-to-be-merged-when-one-has-an-undef-inpu.patch>
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130610/d5c83f8b/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Allow-blocks-to-be-merged-when-one-has-an-undef-inpu.patch
Type: application/octet-stream
Size: 16602 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130610/d5c83f8b/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130610/d5c83f8b/attachment-0001.html>