RFR: 8301584: Generational ZGC: Add barrier elision tests
This changeset adds test cases exercising C2's barrier elision on combinations of different - pairs of dominating / dominated accesses (allocations, loads, stores, atomic operations), - control structures (local, conditions, loops), - object types (arrays, class instances), and - levels of information available during C2's analysis (known vs. unknown array indices). The changeset exposes node barrier data to the [IR test framework](https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/lib/i...) by including it in the node's dump output. This extension is useful not just for testing but also for debugging purposes, for example when exploring C2's intermediate representation with IGV:  The following tests illustrate a missing optimization in C2's barrier elision (reported in [JDK-8301769](https://bugs.openjdk.org/browse/JDK-8301769)), where barriers that should be elided are not: - `testStoreThenAtomic` - `testAtomicThenLoad` - `testAtomicThenStore` - `testAtomicThenAtomic` The changeset does not contain IR checks expecting elision in these cases, to reduce CI pipeline noise. The tests are restricted by now to x64, since volatile loads and stores in other platforms suffer from the same issue (see [JDK-8301769](https://bugs.openjdk.org/browse/JDK-8301769)). **Testing:** tier1-7 (windows-x64, linux-x64, macosx-x64; release and debug mode) ------------- Commit messages: - Restrict tests to x64 by now - Match also aarch64 memory access node names - Remove unnecessary keywords - Remove failing IR checks to avoid CI pipeline noise - Update copyright years - Remove unused arguments - Simplify tests - Complete atomic operation tests - Replace uses of Unsafe with var handles - Add atomic operation tests - ... and 10 more: https://git.openjdk.org/zgc/compare/18e1e84c...3a5ef481 Changes: https://git.openjdk.org/zgc/pull/12/files Webrev: https://webrevs.openjdk.org/?repo=zgc&pr=12&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8301584 Stats: 420 lines in 6 files changed: 417 ins; 0 del; 3 mod Patch: https://git.openjdk.org/zgc/pull/12.diff Fetch: git fetch https://git.openjdk.org/zgc pull/12/head:pull/12 PR: https://git.openjdk.org/zgc/pull/12
On Fri, 3 Feb 2023 15:19:48 GMT, Roberto Castañeda Lozano <rcastanedalo@openjdk.org> wrote:
This changeset adds test cases exercising C2's barrier elision on combinations of different
- pairs of dominating / dominated accesses (allocations, loads, stores, atomic operations), - control structures (local, conditions, loops), - object types (arrays, class instances), and - levels of information available during C2's analysis (known vs. unknown array indices).
The changeset exposes node barrier data to the [IR test framework](https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/lib/i...) by including it in the node's dump output. This extension is useful not just for testing but also for debugging purposes, for example when exploring C2's intermediate representation with IGV:
The following tests illustrate a missing optimization in C2's barrier elision (reported in [JDK-8301769](https://bugs.openjdk.org/browse/JDK-8301769)), where barriers that should be elided are not:
- `testStoreThenAtomic` - `testAtomicThenLoad` - `testAtomicThenStore` - `testAtomicThenAtomic`
The changeset does not contain IR checks expecting elision in these cases, to reduce CI pipeline noise. The tests are restricted by now to x64, since volatile loads and stores in other platforms suffer from the same issue (see [JDK-8301769](https://bugs.openjdk.org/browse/JDK-8301769)).
**Testing:** tier1-7 (windows-x64, linux-x64, macosx-x64; release and debug mode)
Nice test! Looks good. ------------- Marked as reviewed by eosterlund (Reviewer). PR: https://git.openjdk.org/zgc/pull/12
On Fri, 3 Feb 2023 15:19:48 GMT, Roberto Castañeda Lozano <rcastanedalo@openjdk.org> wrote:
This changeset adds test cases exercising C2's barrier elision on combinations of different
- pairs of dominating / dominated accesses (allocations, loads, stores, atomic operations), - control structures (local, conditions, loops), - object types (arrays, class instances), and - levels of information available during C2's analysis (known vs. unknown array indices).
The changeset exposes node barrier data to the [IR test framework](https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/lib/i...) by including it in the node's dump output. This extension is useful not just for testing but also for debugging purposes, for example when exploring C2's intermediate representation with IGV:
The following tests illustrate a missing optimization in C2's barrier elision (reported in [JDK-8301769](https://bugs.openjdk.org/browse/JDK-8301769)), where barriers that should be elided are not:
- `testStoreThenAtomic` - `testAtomicThenLoad` - `testAtomicThenStore` - `testAtomicThenAtomic`
The changeset does not contain IR checks expecting elision in these cases, to reduce CI pipeline noise. The tests are restricted by now to x64, since volatile loads and stores in other platforms suffer from the same issue (see [JDK-8301769](https://bugs.openjdk.org/browse/JDK-8301769)).
**Testing:** tier1-7 (windows-x64, linux-x64, macosx-x64; release and debug mode)
Thanks for reviewing, Erik! ------------- PR: https://git.openjdk.org/zgc/pull/12
On Fri, 3 Feb 2023 15:19:48 GMT, Roberto Castañeda Lozano <rcastanedalo@openjdk.org> wrote:
This changeset adds test cases exercising C2's barrier elision on combinations of different
- pairs of dominating / dominated accesses (allocations, loads, stores, atomic operations), - control structures (local, conditions, loops), - object types (arrays, class instances), and - levels of information available during C2's analysis (known vs. unknown array indices).
The changeset exposes node barrier data to the [IR test framework](https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/lib/i...) by including it in the node's dump output. This extension is useful not just for testing but also for debugging purposes, for example when exploring C2's intermediate representation with IGV:
The following tests illustrate a missing optimization in C2's barrier elision (reported in [JDK-8301769](https://bugs.openjdk.org/browse/JDK-8301769)), where barriers that should be elided are not:
- `testStoreThenAtomic` - `testAtomicThenLoad` - `testAtomicThenStore` - `testAtomicThenAtomic`
The changeset does not contain IR checks expecting elision in these cases, to reduce CI pipeline noise. The tests are restricted by now to x64, since volatile loads and stores in other platforms suffer from the same issue (see [JDK-8301769](https://bugs.openjdk.org/browse/JDK-8301769)).
**Testing:** tier1-7 (windows-x64, linux-x64, macosx-x64; release and debug mode)
Looks good. Interesting test framework, good to know this exists. We should upstream the `src/hotspot/share/gc/shared/c2/barrierSetC2.hpp` and `src/hotspot/share/opto/machnode.cpp` changes test/hotspot/jtreg/compiler/gcbarriers/TestZGCBarrierElision.java line 329:
327: 328: @Test 329: // The second atomic access barrier should be elided, but is not.
Is it worth explicitly documenting the behaviour and the wanted/expected behaviour in annotations? E.g. @Test @IR(counts = { IRNode.Z_GET_AND_SET_P_WITH_BARRIER_FLAG, REMAINING, "2" }, phase = CompilePhase.FINAL_CODE) @IR(counts = { IRNode.Z_GET_AND_SET_P_WITH_BARRIER_FLAG, ELIDED, "0" }, phase = CompilePhase.FINAL_CODE) // The second atomic access barrier should be elided, but is not. // @IR(counts = { IRNode.Z_GET_AND_SET_P_WITH_BARRIER_FLAG, REMAINING, "1" }, phase = CompilePhase.FINAL_CODE) // @IR(counts = { IRNode.Z_GET_AND_SET_P_WITH_BARRIER_FLAG, ELIDED, "1" }, phase = CompilePhase.FINAL_CODE) static void testAtomicThenAtomic(Outer o, Inner i) { field1VarHandle.getAndSet(o, i); field1VarHandle.getAndSet(o, i); } Same goes for all the other test cases. ------------- Marked as reviewed by aboldtch (Committer). PR: https://git.openjdk.org/zgc/pull/12
On Mon, 6 Feb 2023 09:42:43 GMT, Axel Boldt-Christmas <aboldtch@openjdk.org> wrote:
Roberto Castañeda Lozano has updated the pull request incrementally with two additional commits since the last revision:
- Add missing include - Fix include order
test/hotspot/jtreg/compiler/gcbarriers/TestZGCBarrierElision.java line 329:
327: 328: @Test 329: // The second atomic access barrier should be elided, but is not.
Is it worth explicitly documenting the behaviour and the wanted/expected behaviour in annotations? E.g.
@Test @IR(counts = { IRNode.Z_GET_AND_SET_P_WITH_BARRIER_FLAG, REMAINING, "2" }, phase = CompilePhase.FINAL_CODE) @IR(counts = { IRNode.Z_GET_AND_SET_P_WITH_BARRIER_FLAG, ELIDED, "0" }, phase = CompilePhase.FINAL_CODE) // The second atomic access barrier should be elided, but is not. // @IR(counts = { IRNode.Z_GET_AND_SET_P_WITH_BARRIER_FLAG, REMAINING, "1" }, phase = CompilePhase.FINAL_CODE) // @IR(counts = { IRNode.Z_GET_AND_SET_P_WITH_BARRIER_FLAG, ELIDED, "1" }, phase = CompilePhase.FINAL_CODE) static void testAtomicThenAtomic(Outer o, Inner i) { field1VarHandle.getAndSet(o, i); field1VarHandle.getAndSet(o, i); }
Same goes for all the other test cases.
IR checks in compiler tests are commonly interpreted as documentation for expected behavior only. I fear adding IR checks for the current (unexpected) behavior would risk misleading inattentive readers, so I would rather leave it as-is if you don't mind. ------------- PR: https://git.openjdk.org/zgc/pull/12
On Fri, 3 Feb 2023 15:19:48 GMT, Roberto Castañeda Lozano <rcastanedalo@openjdk.org> wrote:
This changeset adds test cases exercising C2's barrier elision on combinations of different
- pairs of dominating / dominated accesses (allocations, loads, stores, atomic operations), - control structures (local, conditions, loops), - object types (arrays, class instances), and - levels of information available during C2's analysis (known vs. unknown array indices).
The changeset exposes node barrier data to the [IR test framework](https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/lib/i...) by including it in the node's dump output. This extension is useful not just for testing but also for debugging purposes, for example when exploring C2's intermediate representation with IGV:
The following tests illustrate a missing optimization in C2's barrier elision (reported in [JDK-8301769](https://bugs.openjdk.org/browse/JDK-8301769)), where barriers that should be elided are not:
- `testStoreThenAtomic` - `testAtomicThenLoad` - `testAtomicThenStore` - `testAtomicThenAtomic`
The changeset does not contain IR checks expecting elision in these cases, to reduce CI pipeline noise. The tests are restricted by now to x64, since volatile loads and stores in other platforms suffer from the same issue (see [JDK-8301769](https://bugs.openjdk.org/browse/JDK-8301769)).
**Testing:** tier1-7 (windows-x64, linux-x64, macosx-x64; release and debug mode)
src/hotspot/share/gc/shared/c2/barrierSetC2.hpp line 33:
31: #include "opto/matcher.hpp" 32: #include "opto/memnode.hpp" 33: #include "opto/machnode.hpp"
Include order src/hotspot/share/opto/machnode.cpp line 26:
24: 25: #include "precompiled.hpp" 26: #include "gc/shared/barrierSet.hpp"
Seems to need `#include "gc/shared/c2/barrierSetC2.hpp"` on some platforms (just looking at your GHA) ------------- PR: https://git.openjdk.org/zgc/pull/12
On Mon, 6 Feb 2023 10:58:21 GMT, Axel Boldt-Christmas <aboldtch@openjdk.org> wrote:
Roberto Castañeda Lozano has updated the pull request incrementally with two additional commits since the last revision:
- Add missing include - Fix include order
src/hotspot/share/gc/shared/c2/barrierSetC2.hpp line 33:
31: #include "opto/matcher.hpp" 32: #include "opto/memnode.hpp" 33: #include "opto/machnode.hpp"
Include order
Done.
src/hotspot/share/opto/machnode.cpp line 26:
24: 25: #include "precompiled.hpp" 26: #include "gc/shared/barrierSet.hpp"
Seems to need `#include "gc/shared/c2/barrierSetC2.hpp"` on some platforms (just looking at your GHA)
Good catch, done! ------------- PR: https://git.openjdk.org/zgc/pull/12
This changeset adds test cases exercising C2's barrier elision on combinations of different
- pairs of dominating / dominated accesses (allocations, loads, stores, atomic operations), - control structures (local, conditions, loops), - object types (arrays, class instances), and - levels of information available during C2's analysis (known vs. unknown array indices).
The changeset exposes node barrier data to the [IR test framework](https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/lib/i...) by including it in the node's dump output. This extension is useful not just for testing but also for debugging purposes, for example when exploring C2's intermediate representation with IGV:
The following tests illustrate a missing optimization in C2's barrier elision (reported in [JDK-8301769](https://bugs.openjdk.org/browse/JDK-8301769)), where barriers that should be elided are not:
- `testStoreThenAtomic` - `testAtomicThenLoad` - `testAtomicThenStore` - `testAtomicThenAtomic`
The changeset does not contain IR checks expecting elision in these cases, to reduce CI pipeline noise. The tests are restricted by now to x64, since volatile loads and stores in other platforms suffer from the same issue (see [JDK-8301769](https://bugs.openjdk.org/browse/JDK-8301769)).
**Testing:** tier1-7 (windows-x64, linux-x64, macosx-x64; release and debug mode)
Roberto Castañeda Lozano has updated the pull request incrementally with two additional commits since the last revision: - Add missing include - Fix include order ------------- Changes: - all: https://git.openjdk.org/zgc/pull/12/files - new: https://git.openjdk.org/zgc/pull/12/files/3a5ef481..27f04950 Webrevs: - full: https://webrevs.openjdk.org/?repo=zgc&pr=12&range=01 - incr: https://webrevs.openjdk.org/?repo=zgc&pr=12&range=00-01 Stats: 3 lines in 2 files changed: 2 ins; 1 del; 0 mod Patch: https://git.openjdk.org/zgc/pull/12.diff Fetch: git fetch https://git.openjdk.org/zgc pull/12/head:pull/12 PR: https://git.openjdk.org/zgc/pull/12
On Wed, 8 Feb 2023 09:05:06 GMT, Roberto Castañeda Lozano <rcastanedalo@openjdk.org> wrote:
This changeset adds test cases exercising C2's barrier elision on combinations of different
- pairs of dominating / dominated accesses (allocations, loads, stores, atomic operations), - control structures (local, conditions, loops), - object types (arrays, class instances), and - levels of information available during C2's analysis (known vs. unknown array indices).
The changeset exposes node barrier data to the [IR test framework](https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/lib/i...) by including it in the node's dump output. This extension is useful not just for testing but also for debugging purposes, for example when exploring C2's intermediate representation with IGV:
The following tests illustrate a missing optimization in C2's barrier elision (reported in [JDK-8301769](https://bugs.openjdk.org/browse/JDK-8301769)), where barriers that should be elided are not:
- `testStoreThenAtomic` - `testAtomicThenLoad` - `testAtomicThenStore` - `testAtomicThenAtomic`
The changeset does not contain IR checks expecting elision in these cases, to reduce CI pipeline noise. The tests are restricted by now to x64, since volatile loads and stores in other platforms suffer from the same issue (see [JDK-8301769](https://bugs.openjdk.org/browse/JDK-8301769)).
**Testing:** tier1-7 (windows-x64, linux-x64, macosx-x64; release and debug mode)
Roberto Castañeda Lozano has updated the pull request incrementally with two additional commits since the last revision:
- Add missing include - Fix include order
Thanks for reviewing, Axel! I just addressed your feedback, [GHA builds](https://github.com/robcasloz/zgc/actions/runs/4122408512/jobs/7120298560) look good now. Please let me know if there is anything else that should be done before integrating. ------------- PR: https://git.openjdk.org/zgc/pull/12
On Wed, 8 Feb 2023 09:05:06 GMT, Roberto Castañeda Lozano <rcastanedalo@openjdk.org> wrote:
This changeset adds test cases exercising C2's barrier elision on combinations of different
- pairs of dominating / dominated accesses (allocations, loads, stores, atomic operations), - control structures (local, conditions, loops), - object types (arrays, class instances), and - levels of information available during C2's analysis (known vs. unknown array indices).
The changeset exposes node barrier data to the [IR test framework](https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/lib/i...) by including it in the node's dump output. This extension is useful not just for testing but also for debugging purposes, for example when exploring C2's intermediate representation with IGV:
The following tests illustrate a missing optimization in C2's barrier elision (reported in [JDK-8301769](https://bugs.openjdk.org/browse/JDK-8301769)), where barriers that should be elided are not:
- `testStoreThenAtomic` - `testAtomicThenLoad` - `testAtomicThenStore` - `testAtomicThenAtomic`
The changeset does not contain IR checks expecting elision in these cases, to reduce CI pipeline noise. The tests are restricted by now to x64, since volatile loads and stores in other platforms suffer from the same issue (see [JDK-8301769](https://bugs.openjdk.org/browse/JDK-8301769)).
**Testing:** tier1-7 (windows-x64, linux-x64, macosx-x64; release and debug mode)
Roberto Castañeda Lozano has updated the pull request incrementally with two additional commits since the last revision:
- Add missing include - Fix include order
Ship it! ------------- PR: https://git.openjdk.org/zgc/pull/12
On Fri, 3 Feb 2023 15:19:48 GMT, Roberto Castañeda Lozano <rcastanedalo@openjdk.org> wrote:
This changeset adds test cases exercising C2's barrier elision on combinations of different
- pairs of dominating / dominated accesses (allocations, loads, stores, atomic operations), - control structures (local, conditions, loops), - object types (arrays, class instances), and - levels of information available during C2's analysis (known vs. unknown array indices).
The changeset exposes node barrier data to the [IR test framework](https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/lib/i...) by including it in the node's dump output. This extension is useful not just for testing but also for debugging purposes, for example when exploring C2's intermediate representation with IGV:
The following tests illustrate a missing optimization in C2's barrier elision (reported in [JDK-8301769](https://bugs.openjdk.org/browse/JDK-8301769)), where barriers that should be elided are not:
- `testStoreThenAtomic` - `testAtomicThenLoad` - `testAtomicThenStore` - `testAtomicThenAtomic`
The changeset does not contain IR checks expecting elision in these cases, to reduce CI pipeline noise. The tests are restricted by now to x64, since volatile loads and stores in other platforms suffer from the same issue (see [JDK-8301769](https://bugs.openjdk.org/browse/JDK-8301769)).
**Testing:** tier1-7 (windows-x64, linux-x64, macosx-x64; release and debug mode)
This pull request has now been integrated. Changeset: c7a20016 Author: Roberto Castañeda Lozano <rcastanedalo@openjdk.org> URL: https://git.openjdk.org/zgc/commit/c7a200169c0aeb4a86bcdd08fa814efd1fb6de44 Stats: 421 lines in 6 files changed: 418 ins; 0 del; 3 mod 8301584: Generational ZGC: Add barrier elision tests Reviewed-by: eosterlund, aboldtch ------------- PR: https://git.openjdk.org/zgc/pull/12
participants (3)
-
Axel Boldt-Christmas
-
Erik Österlund
-
Roberto Castañeda Lozano