Reviewed.
I wish we had a less ambiguous way to mark the overloaded 'If' nodes.
I suggest adding a comment linking your change to IfNode and the concept of smearing:
+ // If this is a range check (IfNode::is_range_check), do not reorder because
+ // Compile::allow_range_check_smearing might have changed the check.
return; // Let IGVN transformation change control dependence.
I suggest adding comments to the test case so that it can be read without referring to the bug report.
Perhaps (although I'm not certain my comments are correct!):
38 if (array[i] < 0) { // range check (i+0) dominates equivalent check below
49.9 // IfNode::Ideal will rewrite some range checks if Compile::allow_range_check_smearing
50 if (array[i-1] == 9) { // range check (i-1) changed to off_lo = (i-3)
51 int res = array[i-3]; // range check (i-3) changed to off_hi = (i+0)
51.1 // the previous access might be hoisted by PhaseIdealLoop::split_if_with_blocks_post
51.2 // because it appears to have the same guard, but it also depends on the previous guard
52 res += array[i]; // removed redundant range check
53 res += array[i-2]; // removed redundant range check
54 return res;
55 }
> On Dec 1, 2014, at 7:11 AM, Roland Westrelin <roland.westrelin at oracle.com> wrote:
>
> http://cr.openjdk.java.net/~roland/8048170/webrev.00/
>
> After range check smearing, an array access may depend on multiple range checks to be valid. PhaseIdealLoop::split_if_with_blocks_post() shouldn’t replace an IF by a dominated IF for the same test for range checks otherwise it can make an array access bypass a range check that it depends on.
>
> Roland.