RFR: 8320718: C2: comparison folding disregards pinned stores
Roberto Castañeda Lozano
rcastanedalo at openjdk.org
Wed Mar 27 10:10:43 UTC 2024
C2 [tries to fold](https://github.com/openjdk/jdk/blob/8fc9097b3720314ef7efaf1f3ac31898c8d6ca19/src/hotspot/share/opto/ifnode.cpp#L1326-L1369) pairs of range check-like comparisons into single unsigned comparisons. In [the case](https://github.com/openjdk/jdk/blob/8fc9097b3720314ef7efaf1f3ac31898c8d6ca19/src/hotspot/share/opto/ifnode.cpp#L1356-L1365) where there is a third, unrelated comparison in between the two folding candidates, this optimization does not take into account whether there is any node (e.g. a store) pinned to the successful projection of the dominating candidate. This results in C2 folding the comparisons, and the pinned nodes being allowed to be hoisted above the original comparisons, which can lead to miscompilations. See further details in the [JBS issue description](https://bugs.openjdk.org/browse/JDK-8320718).
This changeset ensures that two comparisons are not folded when there are nodes pinned to the successful projection of the dominating comparison, regardless of whether the two comparisons are [consecutive](https://github.com/openjdk/jdk/blob/8fc9097b3720314ef7efaf1f3ac31898c8d6ca19/src/hotspot/share/opto/ifnode.cpp#L1331-L1347) or [separated](https://github.com/openjdk/jdk/blob/8fc9097b3720314ef7efaf1f3ac31898c8d6ca19/src/hotspot/share/opto/ifnode.cpp#L1348-L1366) by a third, unrelated test.
The changeset adds negative IR tests checking that comparisons are not folded in the scenario described above, and that, consequently, stores are not hoisted above the comparisons. It also includes the simplified reproducer contributed by @TobiHartmann and a variant of it that reproduces the issue using the default GCM heuristics.
#### Testing
- tier1-5, stress test, fuzzing (windows-x64, linux-x64, linux-aarch64, macosx-x64, macosx-aarch64; release and debug mode).
- Tested that, with the changeset, the original JavaFuzzer-generated test does not fail on 1000 `StressGCM` runs (the failure frequency without the changeset is higher than 50%).
- Tested manually that the changeset does not affect the optimizations applied to the `compiler/rangechecks/TestExplicitRangeChecks.java` tests ([this starter RFE](https://bugs.openjdk.org/browse/JDK-8329101) proposes automating this task by adding IR checks to the tests).
- Tested that the changeset does not introduce performance regressions on a set of standard benchmark suites (DaCapo, SPECjbb2015, SPECjvm2008).
-------------
Commit messages:
- Remove TODO note
- Update copyright years
- Add original test case and a variant that always fails
- Fix class name in IR test
- Disable if-folding if the dominating if has pinned nodes
- Add a couple of negative IR tests
Changes: https://git.openjdk.org/jdk/pull/18506/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18506&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8320718
Stats: 185 lines in 4 files changed: 181 ins; 0 del; 4 mod
Patch: https://git.openjdk.org/jdk/pull/18506.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18506/head:pull/18506
PR: https://git.openjdk.org/jdk/pull/18506
More information about the hotspot-compiler-dev
mailing list