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

[Fei Gao]

On Wed, 12 Oct 2022 13:33:42 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> 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.

@chhagedorn thanks for your review and comments! I updated the commit to resolve the code style issue.

> 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);

Done. Thanks.

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

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