RFR: 8295407: C2 crash: Error: ShouldNotReachHere() in multiple vector tests with -XX:-MonomorphicArrayCheck -XX:-UncommonNullCast [v2]

[Fei Gao]

On Wed, 16 Nov 2022 02:06:36 GMT, Fei Gao <fgao at openjdk.org> wrote:

>> For unsupported `CMove` patterns, [JDK-8293833](https://bugs.openjdk.org/browse/JDK-8295407) helps remove unused `CMove` and related packs from superword candidate packset by the function `remove_cmove_and_related_packs()`, but it only works when `-XX:+UseVectorCmov` is enabled[1]. When the option is not enabled, these unsupported `CMove` packs are still kept in the superword packset, causing the same failure.
>> 
>> Actually, the function `filter_packs()` in superword is to filter out unsupported packs but it can't work as expected currently for these `CMove` cases. As we know, not all `CMove` packs can be vectorized. `merge_packs_to_cmovd()`[2] looks through all packs in the superword packset and generates a `CMove` candidate packset to collect all qualified `CMove` packs. Hence, only `CMove` packs in the `CMove` candidate packset are our target patterns and can be vectorized. But `filter_packs()` thinks, if the `CMove` pack is in a superword packset and its vector node is implemented in the current platform, then it can be vectorized. Therefore, the function doesn't remove these unsupported packs.
>> 
>> We can adjust the function `implemented()` in the stage of `filter_packs()` to check if the current `CMove` pack is in the `CMove` candidate packset. If not, `filter_packs()` considers it not to be vectorized and then remove it. After the fix, whether
>> `-XX:+UseVectorCmov` is enabled or not, these unsupported packs can be removed by `filter_packs()`. In this way, we don't need the function`remove_cmove_and_related_packs()` anymore and thus the patch also cleans related code.
>> 
>> [1] https://github.com/openjdk/jdk/blob/9b9be88bcaa35c89b6915ff0c251e5a04b10b330/src/hotspot/share/opto/superword.cpp#L537
>> [2] https://github.com/openjdk/jdk/blob/9b9be88bcaa35c89b6915ff0c251e5a04b10b330/src/hotspot/share/opto/superword.cpp#L1892
>
> Fei Gao has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Clean up related code
>  - Merge branch 'master' into fg8295407
>  - 8295407: C2 crash: Error: ShouldNotReachHere() in multiple vector tests with -XX:-MonomorphicArrayCheck -XX:-UncommonNullCast
>    
>    For unsupported `CMove` patterns, JDK-8293833 helps remove unused
>    `CMove` and related packs from superword candidate packset by the
>    function `remove_cmove_and_related_packs()`, but it only works
>    when `-XX:+UseVectorCmov` is enabled[1]. When the option is not
>    enabled, these unsupported `CMove` packs are still kept in the
>    superword packset, causing the same failure.
>    
>    Actually, the function `filter_packs()` in superword is to filter
>    out unsupported packs but it can't work as expected currently for
>    these `CMove` cases. As we know, not all `CMove` packs can be
>    vectorized. `merge_packs_to_cmovd()`[2] looks through all packs
>    in the superword packset and generates a `CMove` candidate
>    packset to collect all qualified `CMove` packs. Hence, only
>    `CMove` packs in the `CMove` candidate packset are our target
>    patterns and can be vectorized. But `filter_packs()` thinks,
>    if the `CMove` pack is in a superword packset and its vector
>    node is implemented in the current platform, then it can
>    be vectorized. Therefore, the function doesn't remove
>    these unsupported packs.
>    
>    We can adjust the function `implemented()` in the stage of
>    `filter_packs()` to check if the current `CMove` pack is in
>    the `CMove` candidate packset. If not, `filter_packs()` considers
>    it not to be vectorized and then remove it. After the fix,
>    whether `-XX:+UseVectorCmov` is enabled or not, these
>    unsupported packs can be removed by `filter_packs()`. In this
>    way, we don't need the function`remove_cmove_and_related_packs()`
>    anymore and thus the patch also cleans related code.
>    
>    [1] https://github.com/openjdk/jdk/blob/9b9be88bcaa35c89b6915ff0c251e5a04b10b330/src/hotspot/share/opto/superword.cpp#L537
>    [2] https://github.com/openjdk/jdk/blob/9b9be88bcaa35c89b6915ff0c251e5a04b10b330/src/hotspot/share/opto/superword.cpp#L1892
>    [3] https://github.com/openjdk/jdk/blob/9b9be88bcaa35c89b6915ff0c251e5a04b10b330/src/hotspot/share/opto/superword.cpp#L2701

> I have request since you are touching this code. [8192846](https://bugs.openjdk.org/browse/JDK-8192846) changes were a little sloppy and did not rename some methods which causing confusion. Please, rename `is_CmpD_candidate`, `merge_packs_to_cmpd` and others you find to general `fp` as you did with `is_cmove_fo_opcode`.
> Or may be simply remove `D`: `is_Cmp_candidate(), merge_packs_to_cmove(), test_cmp_pack()`. There are also comments which describes only `CMoveD`.

@vnkozlov, thanks for point it out. I cleaned up related code and comments in the new commit. Could you please help review it? Thanks!


> Do we have IR tests to verify cmove vectorization?

Yes, we do. I added [compiler/c2/irTests/TestVectorConditionalMove.java](https://github.com/openjdk/jdk/blob/master/test/hotspot/jtreg/compiler/c2/irTests/TestVectorConditionalMove.java) in [JDK-8289422](https://bugs.openjdk.org/browse/JDK-8289422).

-------------

PR: https://git.openjdk.org/jdk/pull/11034