RFR: 8293833: Error mixing types with -XX:+UseCMoveUnconditionally -XX:+UseVectorCmov [v2]

[Christian Hagedorn]

On Wed, 12 Oct 2022 01:54:15 GMT, Fei Gao <fgao at openjdk.org> wrote:

>> After JDK-8139340, JDK-8192846 and JDK-8289422, we can vectorize
>> the case below by enabling -XX:+UseCMoveUnconditionally and
>> -XX:+UseVectorCmov:
>> 
>> // double[] a, double[] b, double[] c;
>> for (int i = 0; i < a.length; i++) {
>>     c[i] = (a[i] > b[i]) ? a[i] : b[i];
>> }
>> 
>> 
>> But we don't support the case like:
>> 
>> // double[] a;
>> // int seed;
>> for (int i = 0; i < a.length; i++) {
>>     a[i] = (i % 2 == 0) ? seed + i : seed - i;
>> }
>> 
>> because the IR nodes for the CMoveD in the loop is:
>> 
>>   AddI  AndI     AddD   SubD
>>      \  /         /     /
>>      CmpI        /    /
>>        \        /   /
>>       Bool     /  /
>>           \   / /
>>           CMoveD
>> 
>> 
>> and it is not our target pattern, which requires that the inputs
>> of Cmp node must be the same as the inputs of CMove node
>> as commented in CMoveKit::make_cmovevd_pack(). Because
>> we can't vectorize the CMoveD pack, we shouldn't vectorize
>> its inputs, AddD and SubD. But the current function
>> CMoveKit::make_cmovevd_pack() doesn't clear the unqualified
>> CMoveD pack from the packset. In this way, superword wrongly
>> vectorizes AddD and SubD. Finally, we get a scalar CMoveD
>> node with two vector inputs, AddVD and SubVD, which has
>> wrong mixing types, then the assertion fails.
>> 
>> To fix it, we need to remove the unvectorized CMoveD pack
>> from the packset and clear related map info.
>
> 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:
> 
>  - Refine the function and clean up the code style
>  - Merge branch 'master' into fg8293833
>  - 8293833: Error mixing types with -XX:+UseCMoveUnconditionally -XX:+UseVectorCmov
>    
>    After JDK-8139340, JDK-8192846 and JDK-8289422, we can vectorize
>    the case below by enabling -XX:+UseCMoveUnconditionally and
>    -XX:+UseVectorCmov:
>    ```
>    // double[] a, double[] b, double[] c;
>    for (int i = 0; i < a.length; i++) {
>        c[i] = (a[i] > b[i]) ? a[i] : b[i];
>    }
>    ```
>    
>    But we don't support the case like:
>    ```
>    // double[] a;
>    // int seed;
>    for (int i = 0; i < a.length; i++)
>        a[i] = (i % 2 == 0) ? seed + i : seed - i;
>    }
>    ```
>    because the IR nodes for the CMoveD in the loop is:
>    ```
>      AddI  AndI     AddD   SubD
>         \  /         /     /
>         CmpI        /    /
>           \        /   /
>          Bool     /  /
>              \   / /
>              CMoveD
>    ```
>    
>    and it is not our target pattern, which requires that the inputs
>    of Cmp node must be the same as the inputs of CMove node as
>    commented in CMoveKit::make_cmovevd_pack(). Because we can't
>    vectorize the CMoveD pack, we shouldn't vectorize its inputs,
>    AddD and SubD. But the current function
>    CMoveKit::make_cmovevd_pack() doesn't clear the unqualified
>    CMoveD pack from the packset. In this way, superword wrongly
>    vectorizes AddD and SubD. Finally, we get a scalar CMoveD node
>    with two vector inputs, AddVD and SubVD, which has wrong mixing
>    types, then the assertion fails.
>    
>    To fix it, we need to remove the unvectorized CMoveD pack from
>    the packset and clear related map info.

Thanks for doing the update, looks good to me! It's indeed much easier to follow the code now. I'll submit some testing.

src/hotspot/share/opto/superword.cpp line 1936:

> 1934: 
> 1935: bool CMoveKit::is_cmove_pack_candidate(Node_List* cmove_pk) {
> 1936:   Node *cmove = cmove_pk->at(0);

Suggestion:

  Node* cmove = cmove_pk->at(0);

src/hotspot/share/opto/superword.cpp line 1947:

> 1945: // i.e. bool node pack and cmp node pack, can be successfully merged for vectorization.
> 1946: bool CMoveKit::can_merge_cmove_pack(Node_List* cmove_pk) {
> 1947:   Node *cmove = cmove_pk->at(0);

Suggestion:

  Node* cmove = cmove_pk->at(0);

src/hotspot/share/opto/superword.cpp line 1993:

> 1991: // new pack and delete the old cmove pack and related packs from the packset.
> 1992: void CMoveKit::make_cmove_pack(Node_List* cmove_pk) {
> 1993:   Node *cmove = cmove_pk->at(0);

Suggestion:

  Node* cmove = cmove_pk->at(0);

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

Marked as reviewed by chagedorn (Reviewer).

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