RFR: 8316679: C2 SuperWord: wrong result, load should not be moved before store if not comparable
Emanuel Peter
epeter at openjdk.org
Fri Sep 22 13:11:43 UTC 2023
**Context**
In `SuperWord::output`, we replace the scalars with vectors, pack by pack.
In [JDK-8052081](https://bugs.openjdk.org/browse/JDK-8052081) ([changeset](https://github.com/openjdk/jdk/commit/7764490363a5963212e040baa2b79946cf4487bf)) we added an optimization to move the memory state of a LoadVector as early as possible. We go up the memory chain, and skip any `StoreVector` that provably can never alias with the `LoadVector` (provably no overlap).
**Details**
For this, we must understand the following function `VPointer::cmp`:
https://github.com/openjdk/jdk/blob/96781ba33d4717c8df2b9ba4cd5d66858cf5c2d1/src/hotspot/share/opto/vectorization.hpp#L110-L120
We have 4 possible states: `NotComparable` means that we cannot determine the relative offsets between the two memory addresses. We have no guarantee about aliasing at all. `Less` means that one address lies before the other, `Greater` means that it comes after. `Equal` means that the two memory regions have an overlap or are identical. To determine overlap, we must know the position of the pointer, as well as the `memory_size` of each memory reference. `VPointer::not_equal` is true iff we get states `Less` or `Greater` (we know there cannot be overlap). `VPointer::comparable` is true iff the state is `NotComparable`, hence we have no guarantee about aliasing.
**Problem**
Currently, there are 2 bugs in this "move-load-before-store" logic:
1. [JDK-8316679](https://bugs.openjdk.org/browse/JDK-8316679) ([PR](https://github.com/openjdk/jdk/pull/15864)): We do not only skip the `not_equal` but also the `!comparable` case. However, when the two pointers are not comparable, that could be due to different invariants for example (see `TestMovingLoadBeforeStore.java`). The two pointers are not comparable, because we do not know the relative position, due to the invariants. But this exactly means that we cannot know at compile-time that there will not be aliasing. Thus, we cannot allow moving a load before such a store. Otherwise we may get wrong results. (*)
2. [JDK-8316594](https://bugs.openjdk.org/browse/JDK-8316594) ([PR](https://github.com/openjdk/jdk/pull/15866)): Currently, we only compare the first element of the load-pack with the `StoreVector`. This is not correct, we need to compare all elements of the load-pack with the `StoreVector`. Otherwise it may be that the first element lays below/before the memory region of the `StoreVector` (Less), but the last element of the load-pack for example could have overlap with the `StoreVector` (Equal).
We could fix both issues at once. But I am fixing them separately, first JDK-8316679, which can be backported to JDK-11, and then JDK-8316594 which only seems to have caused regressions in JDK-22.
**Testing**
I added correpsonding regression tests.
Tier1-6 + stress-testing (Running)
Performance testing (Running)
---------
(*) We also use `!not_equal` in `SuperWord::dependence_graph` to determine if two memrefs need to have a dependency edge. The condition there and the condition here (for move-load-before-store) should be equivalent.
https://github.com/openjdk/jdk/blob/96781ba33d4717c8df2b9ba4cd5d66858cf5c2d1/src/hotspot/share/opto/superword.cpp#L1112-L1116
-------------
Commit messages:
- beatify code
- flip flag
- fix diagnostic option
- whitespace
- Merge branch 'master' into JDK-8316679
- 8316679: C2 SuperWord: wrong result, load should not be moved before store if not comparable
Changes: https://git.openjdk.org/jdk/pull/15864/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15864&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8316679
Stats: 110 lines in 2 files changed: 103 ins; 4 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/15864.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/15864/head:pull/15864
PR: https://git.openjdk.org/jdk/pull/15864
More information about the hotspot-compiler-dev
mailing list