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

[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 assume than any floating node
that's dependent on a range check or predicate could float too
high. It's both too conservative (only nodes for array loads can float
too high: `CastII` and `Load` nodes and only if smearing or
predication was applied) and too optimistic (it should apply to any
test as demonstrated by the test cases). What I propose instead is
that when range smearing or predication for a range check is applied,
`CastII` and array `Load` nodes are marked as pinned. When a test is
replaced by a dominating one, nodes that need to not float are pinned
and so there's no need for special logic. I also propose that RC
smearing be delayed to after loop opts so nodes are not pinned early
on. I assume the benefit of RC smearing is for sequence of array
accesses that optimize poorly during loop opts.

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

Commit messages:
 - fix & test

Changes: https://git.openjdk.org/jdk/pull/16886/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16886&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8319793
  Stats: 361 lines in 14 files changed: 303 ins; 27 del; 31 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