[llvm-commits] [PATCH] Restructure a part of SimplifyCFG

[Chris Lattner]

On May 16, 2008, at 7:14 AM, Matthijs Kooijman wrote:
> However, in my patch I've restructured the  
> CanPropagatePredecessorsForPHIs
> function to look a lot more similar to the code that actually  
> performs the
> merge. Both functions now look at the same phi nodes in about the  
> same order.
> Any conflicts (phi nodes with different values for the same source)  
> that could
> arise from merging or moving phi nodes are detected. If no conflicts  
> are
> found, the merge can happen.

Very nice, you're right that the old code is quite gross :)

> Apart from only restructuring the checks, two main changes in  
> functionality
> happened.
>
> Firstly, the old code rejected blocks with common predecessors in  
> most cases.
> The new code performs some extra checks so common predecessors can  
> be handled
> in a lot of cases. Wherever common predecessors still pose problems,  
> the
> blocks are left untouched.
>
> Secondly, the old code rejected the merge when values (phi nodes)  
> from BB were
> used in any other place than Succ. However, after giving this some  
> extensive
> thought, I cannot think of any situation that would require this  
> check. Even
> more, I think this can be proven.
>
> Consider that BB is a block containing of a single phi node "%a" and  
> a branch
> to Succ. Now, since the definition of %a will dominate all of its  
> uses, BB
> will dominate all blocks that use %a. Furthermore, since the branch  
> from BB to
> Succ is unconditional, Succ will also dominate all uses of %a.
>
> Now, assume that one predecessor of Succ is not dominated by BB (and  
> thus not
> dominated by Succ). Since at least one use of %a (but in reality all  
> of them)
> is reachable from Succ, you could end up at a use of %a without  
> passing
> through it's definition in BB (by coming from X through Succ). This  
> is a
> contradiction, meaning that our original assumption is wrong. Thus,  
> all
> predecessors of Succ must also be dominated by BB (and thus also by  
> Succ).
>
> This means that moving the phi node %a from BB to Succ does not pose  
> any
> problems when the two blocks are merged, and any use checks are not  
> needed.
>
> I'm also working on a few testcases to properly illustrate the  
> changes by this
> patch, I will send them in a seperate email later today.

The patch looks great to me, if it passes tests, please apply.

-Chris