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

[Mark Lacey]

Ping?

On Jun 28, 2013, at 7:09 PM, Mark Lacey <mark.lacey at apple.com> wrote:
> Hi Duncan,
> 
> On Jun 18, 2013, at 5:58 AM, Duncan Sands <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
> 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/20130703/21881ae5/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Small-refactoring-of-TryToSimplifyUncondBranchFromEm.patch
Type: application/octet-stream
Size: 4050 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130703/21881ae5/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130703/21881ae5/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Allow-blocks-to-be-merged-when-one-has-an-undef-inpu.patch
Type: application/octet-stream
Size: 18960 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130703/21881ae5/attachment-0001.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130703/21881ae5/attachment-0002.html>