RFR(S): 8252696: Loop unswitching may cause out of bound array load to be executed

[Roland Westrelin]

http://cr.openjdk.java.net/~roland/8252696/webrev.00/
https://bugs.openjdk.java.net/browse/JDK-8252696

This came up with testing of 8223051 (long counted loops):

https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2020-August/039801.html

Here are the steps that lead to the crash:

The graph includes a counted loop. The loop body contains a LoadP for an
array access. Loop predication optimizes the array access range check
and the LoadP becomes control dependent on a predicate. The loop is
unswitched. 8240227 (Loop predicates should be copied to unswitched
loops) added logic so predicates are cloned for both loops on
unswitching and data nodes are made control dependent on the per branch
predicates. That logic only applies to RangeCheck predicates but in this
case the predicate is an If node because PhaseIdealLoop::rc_predicate()
built it with a CmpUL (to protect from overflow). So after unswitching,
the LoadP is still control dependent on the predicate that was added at
predication time and dominates both loops and the test that triggered
unswitching. A pre/post loop is then created and the main loop is
unrolled a few times. The exit condition of the main loop is proven to
be always false so the loop nodes of the main loop are optimized out but
the main loop body remains. The LoadP nodes (several of them now because
of unrolling) can float above the loop unswitching test which causes and
out of bound array load to be perfomed: the main loop is not executed
but a LoadP that's only valid when in the loop body "espaces" the loop
body.

I couldn't write a test case that reproduces it with current jdk code
but this doesn't seem specific to the long counted loop patch. The root
cause is that 8240227 only considers RangeCheck predicates when it
should consider also If predicates. The current logic copies all
RangeCheck predicates to both unswitched loops and then eliminates the
dominating ones that it finds useless. Removing dominated predicates
that have no control dependent data nodes the way it's done in the
current logic is suspicious: data nodes are sometimes logically
dependent on multiple range checks but control dependent on only one. I
also believe only skeleton predicates need to be copied (so the
skeletons can be expanded when the pre/main/post loops are created and
on unrolling) and that's what I propose in the patch. I also removed the
assert because AFAIU:

new_node != NULL && old_node->_idx >= idx_before_clone

is simply an impossible condition: first part is true if old_node was
cloned to new_node and second part is true if old_node is a clone.

Roland.