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

[Christian Hagedorn]

Hi Roland

Nice analysis! This fix sounds reasonable to me. Have I understood that 
correctly that in your testcase, the main loop is just unrolled enough 
such that the loop nodes can be removed (so no over-unrolling)?

> [..] 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.

That's a valid point. Is there a better way we could find out which 
predicates are useless after cloning them to the slow and fast loop or 
is it either way not a problem to keep the original ones alive?

> 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.

I agree, I think I added this back there because I was temporarily 
modifying the old_new mapping in a non-conform way on L302. So, I just 
wanted to be sure that everything is reset and works as intended. But 
I'm also fine with just removing this assertion code.

Some small comment:
- You probably don't need the assertion on L257 in loopPredicate.cpp as 
you are already checking it in the if-condition on L253.
- Same file, you should update the assert message on L268 to something 
like "... projection of an If node".

Best regards,
Christian

On 03.09.20 10:10, Roland Westrelin wrote:
> 
> 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.
>