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