RFR: 8319793: C2 compilation fails with "Bad graph detected in build_loop_late" after JDK-8279888 [v11]

[Roland Westrelin]

> Range check smearing and range check predication make an array access
> dependent on 2 (or more in the case of RC smearing) conditions. As a
> consequence, if a range check can be eliminated because there's an
> identical dominating range check, the control dependent nodes that
> could float and become dependent on the dominating range check cannot
> be allowed to float because there's a risk that they would then bypass
> one of the checks that make the access legal.
> 
> `IfNode::dominated_by()` and `PhaseIdealLoop::dominated_by()` have
> logic to prevent this: nodes that are control dependent on a range
> check or predicate are not allowed to float. This is however not
> sufficient as demonstrated by the test cases.
> 
> In `TestArrayAccessAboveRCAfterSmearingOrPredication.testRangeCheckSmearing()`:
> 
> 
>                 v += array[i];
>                 if (flag2) {
>                     if (flag3) {
>                         field = 0x42;
>                     }
>                 }
>                 if (flagField == 1) {
>                     v += array[i];
>                 }
> 
> 
> The range check for the second `array[i]` load is replaced by the
> dominating range check for the first `array[i]` but because the second
> `array[i]` load could really be dependent on multiple range checks (in
> case smearing happened which is not the case here), c2 doesn't allow
> the second `array[i]` to float when the second range check is
> removed. The second `array[i]` is then control dependent on:
> 
> 
>                 if (flagField == 1) {
> 
> 
> which is next found to be dominated by the same test:
> 
> 
>         if (flag == 1) {
> 
> 
> and is removed. However nothing in `dominated_by()` treats node
> dependent on tests that are not range check or predicates
> specially. So the second `array[i]` is allowed to float and become
> dependent on:
> 
> 
>         if (flag == 1) {
> 
> 
> which is above the range check for that access. The test method in its
> last invocation is passed an index for the array access that's widely
> out of range. The array load happens before the range check and
> crashes the VM. `testLoopPredication()` is a similar test where array
> loads become dependent on predicates and end up above range checks.
> 
> `TestArrayAccessCastIIAboveRC.java` is the test case from the bug
> where for similar reasons a range check `CastII` ends up above its
> range check, becomes top because its input becomes some integer that
> conflicts with its type (but there's no condition to catch it). The
> graph becomes broken and c2 crashes.
> 
> Logic in the `dominated_by()` methods ...

Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:

  whitespaces

-------------

Changes:
  - all: https://git.openjdk.org/jdk/pull/16886/files
  - new: https://git.openjdk.org/jdk/pull/16886/files/2cc6f1d3..51231631

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16886&range=10
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16886&range=09-10

  Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 mod
  Patch: https://git.openjdk.org/jdk/pull/16886.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16886/head:pull/16886

PR: https://git.openjdk.org/jdk/pull/16886