[jdk18] Integrated: 8279654: jdk/incubator/vector/Vector256ConversionTests.java crashes randomly with SVE
Fei Gao
fgao at openjdk.java.net
Wed Jan 19 01:24:32 UTC 2022
On Thu, 13 Jan 2022 06:34:18 GMT, Fei Gao <fgao at openjdk.org> wrote:
> While testing some vector api cases on SVE system, we see a random failure:
> "assert(C->node_arena()->contains(s->_leaf) || !has_new_node (s->_leaf)) failed: duplicating node that's already been matched".
>
> The root cause is that the if-condition in pd_clone_node()[1] is not aligned with predicate conditions[2] of match rules in the backend. Not all StoreVector (VectorStoreMask src) node patterns can be matched and subsumed into one instruction in the backend. For example, there is no match rule to combine them when the element basic type is T_BYTE, as the cost of VectorStoreMask for byte type is relatively low and there is no need to do narrow operations.
>
> Here is the analysis about root cause in detail. When a multi-use VectorStoreMask node, whose type is byte, showed as below,
>
> 
>
> is identified by pd_clone_node() successfully, it will not be set shared in the stage of find_shared() if only the root selector visits
> the call node earlier than StoreVector node[3]. The preorder walk in xform()[4][5] visits the VectorStoreMask node and reduces it by ReduceInst() starting from the call node first, then visits the VectorStoreMask node again starting from the StoreVector node as mentioned before. During the second visit, in the stage of Label_Root(), the postorder walk along use-def edges starts from the StoreVector node and then visits the VectorStoreMask node. It takes the VectorStoreMask node as an interior of the subtree and generates the corresponding state tree[6]. But in the stage of ReduceInst_Interior(), there is no internal rule in the backend to reduce the VectorStoreMask node as the state tree guides. Thus, the reducer has to reduce the VectorStoreMask node independently and visit it by ReduceInst()[7]. Therefore, it tries to reduce the VectorStoreMask node twice by ReduceInst(), resulting in the assertion failure. Assuming that there was a match rule to combine these two byte nodes,
> StoreVector (VectorStoreMask src), it could find an internal rule in the backend to help skip the interior VectorStoreMask node, do nothing but recurse[8], and no assertion happens. If we delete the check for VectorStoreMask in pd_clone_node(), the VectorStoreMask node is going to be set shared in find_shared() and could be definitely reduced only once with no assertion failure[9][10].
>
> There are two different methods to fix this issue.
>
> The first one is setting the condition in pd_clone_node() the same as matching rules, making sure that all instruction sequences declared in pd_clone_node() can be subsumed by matching rules. However, the condition code has to be revisited when matching rules change in the future. It's not easy to maintain.
>
> The other one is that we can remove the if-condition code for VectorStoreMask in pd_clone_node(). As a result, when a VectorStoreMask node has multiple use, it can't be matched by any chained rules. But after [JDK-8273949](https://bugs.openjdk.java.net/browse/JDK-8273949), a multi-use VectorStoreMask node only exists for safepoint[11], which is not very common. Furthermore, even if a multi-use VectorStoreMask node occurs, the pattern is identified in the pd_clone_node() and a match rule is ready to subsume it in the backend, the combination may not happen due to some unexpected matching order issue. For example, in this case,
> 
>
> if we visit the StoreVector node first then the call node, the VectorStoreMask node is still going to be set shared and the prevention from pd_clone_node() doesn't work based on the current logic of find_shared()[12]. Once the node is set shared, there is no code path to use internal combination rule to subsume the chained StoreVector-VectorStoreMask nodes into one machine instruction.
>
> Based on the above mentioned considerations, to make the code more maintainable, we choose the second one, which decouples pd_clone_node() from predicate conditions of matching rules in the backend with minimal impact on performance.
>
> I tested the patch using all vector cases under compiler/vectorization, compiler/vectorapi and jdk/incubator/vector on SVE for multiple times. All tests passed.
>
> [1] https://github.com/openjdk/jdk/blob/8d1a1e83f40f7a147e033be6b2221c1bb1abd8ab/src/hotspot/cpu/aarch64/aarch64.ad#L2738
> [2] https://github.com/openjdk/jdk/blob/8d1a1e83f40f7a147e033be6b2221c1bb1abd8ab/src/hotspot/cpu/aarch64/aarch64_sve.ad#L2101
> [3] https://github.com/openjdk/jdk/blob/8d1a1e83f40f7a147e033be6b2221c1bb1abd8ab/src/hotspot/share/opto/matcher.cpp#L2151
> [4] https://github.com/openjdk/jdk/blob/8d1a1e83f40f7a147e033be6b2221c1bb1abd8ab/src/hotspot/share/opto/matcher.cpp#L1097
> [5] https://github.com/openjdk/jdk/blob/8d1a1e83f40f7a147e033be6b2221c1bb1abd8ab/src/hotspot/share/opto/matcher.cpp#L1119
> [6] https://github.com/openjdk/jdk/blob/8d1a1e83f40f7a147e033be6b2221c1bb1abd8ab/src/hotspot/share/opto/matcher.cpp#L1694
> [7] https://github.com/openjdk/jdk/blob/8d1a1e83f40f7a147e033be6b2221c1bb1abd8ab/src/hotspot/share/opto/matcher.cpp#L1977
> [8] https://github.com/openjdk/jdk/blob/8d1a1e83f40f7a147e033be6b2221c1bb1abd8ab/src/hotspot/share/opto/matcher.cpp#L1975
> [9] https://github.com/openjdk/jdk/blob/8d1a1e83f40f7a147e033be6b2221c1bb1abd8ab/src/hotspot/share/opto/matcher.cpp#L1692
> [10] https://github.com/openjdk/jdk/blob/8d1a1e83f40f7a147e033be6b2221c1bb1abd8ab/src/hotspot/share/opto/matcher.cpp#L1969
> [11] https://github.com/openjdk/jdk/blob/8d1a1e83f40f7a147e033be6b2221c1bb1abd8ab/src/hotspot/share/opto/vector.cpp#L262
> [12] https://github.com/openjdk/jdk/blob/8d1a1e83f40f7a147e033be6b2221c1bb1abd8ab/src/hotspot/share/opto/matcher.cpp#L2125
This pull request has now been integrated.
Changeset: af6c9aba
Author: Fei Gao <fgao at openjdk.org>
Committer: Ningsheng Jian <njian at openjdk.org>
URL: https://git.openjdk.java.net/jdk18/commit/af6c9abafaa3b8f9bdcc938fec465aeeed8b30ed
Stats: 5 lines in 1 file changed: 0 ins; 3 del; 2 mod
8279654: jdk/incubator/vector/Vector256ConversionTests.java crashes randomly with SVE
Reviewed-by: njian, kvn
-------------
PR: https://git.openjdk.java.net/jdk18/pull/98
More information about the hotspot-compiler-dev
mailing list