RFR: 8316105: C2: Back to back Parse Predicates from different loops but with same deopt reason are wrongly grouped together
Christian Hagedorn
chagedorn at openjdk.org
Fri Sep 15 14:56:15 UTC 2023
[JDK-8305636](https://bugs.openjdk.org/browse/JDK-8305636) wrongly refactored the code that groups predicates into Predicate Blocks which leads to a wrong execution when hitting a trap.
**Background**
In general, when a loop X is folded and its Parse Predicates end up above loop Y, we can reuse the Parse Predicates from loop X to create Runtime Predicates with for loop Y. If such a Runtime Predicate of loop Y is false during runtime, we trap and jump back to the start of the folded loop X above. This is fine because we have not executed any side effects of loop X or between loop X and Y:
- **(i)** There are no other CFG nodes with possible side effects between loop X and Y. Otherwise, the Parse Predicates of loop X are not directly above loop Y and are removed by `eliminate_useless_parse_predicates()`.
- **(ii)** There can be no stores pinned at Parse Predicates (they are not hoisted out of a loop with range checks and invariant checks) except at the last one. This happens, for example, if loop X is fully unrolled and folded away in IGVN. A store is then no longer pinned at the folded `CountedLoopNode` but at the last Parse Predicate of loop X. If there are no other CFG nodes between loop X and Y, then the Parser Predicates from loop X end up just above loop Y. All Runtime Predicates created for loop Y with Parse Predicates from loop X will be executed before the pinned store at the last Parse Predicate (i.e. the loop entry).
**Problem**
In the test case, we have loop A that is fully unrolled and folded. Its Parse Predicates end up above loop B after IGVN. The only difference to the situation with loop X and Y in **(ii)** is that loop B still has a Loop Limit Check Parse Predicate:
- `40 Parse Predicate` and `51 Parse Predicate` are from loop A with the pinned `80 StoreL` from within the loop body of A (i.e. `lFld`).
- `107 Parse Predicate` is from loop B.
When eliminating useless Parse Predicates, we should remove the Parse Predicates of loop A, because **(i)** is violated: We have a CFG node (e.g. `107 Parse Predicate`). between the loop B and the Parse Predicates of loop A. This, however, does not happen due to a bug: We wrongly group the back to back Loop Limit Check Parse Predicates `107 Parse Predicate` and `51 Parse Predicate` together as single Predicate Block. As a result, we keep `51 Parse Predicate` and `40 Parse Predicate`.
**Manifestation of the Bug**
Keeping the Parse Predicates of loop A for now is not a problem, yet, in the test case. Even if we created a Runtime Predicate with `40 Parse Predicate` that traps at runtime, `80 StoreL` would not have been executed, yet.
The problem happens when unswitching loop B. We clone `41 Parse Predicate` to the slow (i.e. `403 Parse Predicate`) and fast loop (i.e. `400 Parse Predicate`) and kill the old Parse Predicates above the `If` that selects either the slow or fast loop (we don't clone Loop Limit Check Parse Predicates for counted loops because they will not be used anymore at this point).`80 StoreL` then ends up at `5 Parm`. This would still not result in a problem at runtime, yet. But now we additionally create a Hoisted Check Predicate `411 If` for an invariant check above the fast loop. We get the following graph:
At runtime, we execute `80 StoreL`, then hit the trap of `411 If`, jump back to the beginning of loop A and re-execute the store to `lFld`. We therefore observe a wrong value for `lFld` because we executed the store twice.
**Previous Fix for Known Loop Unswitching Problem Does not Apply here**
There is logic introduced by [JDK-8235984](https://bugs.openjdk.org/browse/JDK-8235984) which prevents loop unswitching completely if there are pinned nodes at the last Parse Predicate because we could end up executing such a pinned store before trapping with a Runtime Predicate:
https://github.com/openjdk/jdk/blob/3c743cfea00692d0b938cb1cbde936084eecf369/src/hotspot/share/opto/loopUnswitch.cpp#L206-L215
But since we can normally only have pinned stores at the last Parse Predicate, we do not bail out of loop unswitching (the pin is at the second last Parse Predicate) and we end up with the described wrong execution.
**Fix**
This all comes down to wrongly grouping the back to back Loop Limit Check Parse Predicates together which prevents the elimination of the Parse Predicates of loop A. The actual problem introduced with JDK-8305636 is that a Parse Predicate is wrongly detected as a Runtime Predicate. The fix is straight forward to exclude that case.
Thanks,
Christian
-------------
Commit messages:
- 8316105: C2: Back to back Parse Predicates from different loops but with same deopt reason are wrongly grouped together
Changes: https://git.openjdk.org/jdk/pull/15764/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15764&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8316105
Stats: 86 lines in 2 files changed: 85 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/15764.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/15764/head:pull/15764
PR: https://git.openjdk.org/jdk/pull/15764
More information about the hotspot-compiler-dev
mailing list