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

[Duncan Sands]

Hi Mark,

On 03/07/13 09:36, Mark Lacey wrote:
> Ping?

sorry for the delay, LGTM!  Thanks for doing this, and in particular for adding
all those helpful comments!

Ciao, Duncan.

>
> On Jun 28, 2013, at 7:09 PM, Mark Lacey <mark.lacey at apple.com
> <mailto:mark.lacey at apple.com>> wrote:
>> Hi Duncan,
>>
>> On Jun 18, 2013, at 5:58 AM, Duncan Sands <duncan.sands at gmail.com
>> <mailto:duncan.sands at gmail.com>> wrote:
>>
>>> Hi Mark,
>>>
>>> On 17/06/13 19:41, Mark Lacey wrote:
>>>> Hi All,
>>>>
>>>> This is a relatively small patch I re-submitted last week which seems to
>>>> have gotten lost in the shuffle.
>>>>
>>>> Can someone take a look and commit if everything looks good?
>>>
>>> sorry for forgetting about this.  My only remaining concern is that the
>>> algorithm seems to be quadratic in the number of phi node operands.  It
>>> is possible (though rare) to have phi nodes with hundreds of operands.
>>> Maybe it is better to exploit a container type, eg use a map from basic
>>> block to new value.  Then you can eg just whiz down the existing phi nodes,
>>> chucking their (bb, value) pairs into the map whenever "value" is not undef,
>>> then construct the new phi node by, for each predecessor bb, adding (bb, value)
>>> if bb -> value in the map, and adding (bb, undef) if bb is not in the map.  Or
>>> something like that.
>>
>> Thanks for taking another look. Apologies of my own for not getting back to
>> you sooner, but I am just able to get back to this now.
>>
>> The algorithm isn't quadratic in the number of phi node operands, but rather
>> if the block with the phi has N predecessors, and we are trying to merge that
>> block with a predecessor that itself has M predecessors, we can potentially do
>> work proportional to MxN to determine the final inputs for the phi (which
>> could potentially be as bad or worse than N^2, but could also be quite a bit
>> better). The function CanPropagatePredecessorsForPHIs() does something similar
>> (and gates whether we'll reach this particular code), but it appears to
>> potentially improve on that worst case by using a SmallPtrSet<> to test for a
>> common predecessor between the blocks (which when small has similar behavior,
>> but when large is potentially better depending on how well the hash function
>> performs).
>>
>> I did some testing and found that indeed when merging a block with several
>> hundred predecessors into another block with several hundred predecessors,
>> there is a noticeable slow-down in SimplifyCFG.
>>
>> I like your idea, so I went ahead and implemented it, as well as keeping track
>> of which undef values were found in the phi (and then only walking those again
>> to update them).
> [edit: I meant to revise this line before hitting send - this was an earlier
> draft. Although it might be possible to keep track of the undef values and only
> walk those again to update, it added complexity and did not seem to be
> worthwhile, so instead the phi operands are walked again and updated after
> selecting a value for the undef inputs.]
>>
>> I've attached two patches - the first just refactors the body of the loop that
>> is walking the phis into a separate function, and the second has the
>> functional changes.
>>
>> I built debug & release, and ran check-all and the full test suite.
>>
>> Can you take another look, and if this looks okay, commit it?
>>
>> Thanks again,
>>
>> Mark
>>
>>
>> [PATCH 1/2] Small refactoring of TryToSimplifyUncondBranchFromEmptyBlock.
>>
>> Move the code that updates the phi nodes into a separate function. This
>> is in preparation for some functional changes to that factored-out code.
>> ---
>> lib/Transforms/Utils/Local.cpp | 58 +++++++++++++++++++++++++++---------------
>> 1 file changed, 37 insertions(+), 21 deletions(-)
>>
>>
>
>
>>
>>
>> [PATCH 2/2] Allow blocks to be merged when one has an undef input to a phi.
>>
>> 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                     | 124 ++++++++++-
>> .../SimplifyCFG/2008-05-16-PHIBlockMerge.ll        | 131 -----------
>> .../SimplifyCFG/EqualPHIEdgeBlockMerge.ll          | 240 ++++++++++++++++++++-
>> 3 files changed, 355 insertions(+), 140 deletions(-)
>> delete mode 100644 test/Transforms/SimplifyCFG/2008-05-16-PHIBlockMerge.ll
>>
>>
>
>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>