RFR(S): 8187822: C2 conditonal move optimization might create broken graph

[Vladimir Kozlov]

Thanks, Roland

I agree with your analysis.

But as I understand we can't replace such diamond code with cmove 
because If node will not be eliminated if you not adjust control of 
LoadI node.

What graph you have as result of your changes?

Thanks,
Vladimir

On 9/25/17 7:19 AM, Roland Westrelin wrote:
> 
> http://cr.openjdk.java.net/~roland/8187822/webrev.00/
> 
> PhaseIdealLoop::conditional_move() replaces a if diamond with a CMoveX
> node, sets the control of the CMoveX node right above the if node and
> adjust the control of the inputs of the CMoveX node so it dominates the
> CMoveX node. The bug here is that one input of a CMoveX node can depend
> on another data node whose control is one of the branch of the
> if. PhaseIdealLoop::conditional_move() doesn't adjust the control of
> that node and so loop opts proceed with a node whose control doesn't
> dominate the control of its uses.
> 
> The test case is an example of this. The if (flag2) test is converted to
> a CMoveI. One of its inputs is f + v1: a AddI that has a LoadI
> input. Control of the AddI is set to be right above the if but the
> control of the LoadI is not adjusted and in this case is the if
> branch. In this round of loop opts, the AddI is considered for a split
> thru phi. C2 shouldn't proceed with the split thru phi because its LoadI
> input is not a phi and doesn't strictly dominate the region of the
> phi. The strict domination test is implemented in
> PhaseIdealLoop::has_local_phi_input() as:
> 
> get_ctrl(m) == n_ctrl
> 
> which fails for the LoadI node because its control is below the AddI
> node.
> 
> The fix I suggest is to set the control of the new CMoveX node below the
> if diamond. Then there's no need to fix the control of the inputs of the
> CMoveX.
> 
> Roland.
>