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

[Fei Gao]

On Mon, 10 Oct 2022 08:49:14 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.
>
> src/hotspot/share/opto/superword.cpp line 1981:
> 
>> 1979:   }
>> 1980: 
>> 1981:   Node_List* new_cmpd_pk = new Node_List();
> 
> The following suggestion is just an idea as I was a little bit confused by how you use the return value of `make_cmovevd_pack` to remove the cmove pack and its related packs. Intuitively, I would have expected that this "make method" returns the newly created pack instead. 
> 
> Maybe it's cleaner if you split this method into a "should merge" method with the check
> 
>  if ((cmovd->Opcode() != Op_CMoveF && cmovd->Opcode() != Op_CMoveD) ||
>       pack(cmovd) != NULL /* already in the cmov pack */) {
>     return NULL;
>   }
> 
> a "can merge" method that checks all the other constraints and an actual "make pack" method with the code starting at this line. Then you could use these methods in `merge_packs_to_cmovd` like that in pseudo-code:
> 
> void SuperWord::merge_packs_to_cmovd() {
>   for (int i = _packset.length() - 1; i >= 0; i--) {
>     Node_List* pack = _packset.at(i);
>     if (_cmovev_kit.should_merge(pack)) {
>       if (_cmovev_kit.can_merge(pack)) {
>         _cmovev_kit.make_cmovevd_pack(pack)
>       } else {
>         remove_cmove_and_related_packs(pack);
>       }
>     }
>   }
>   ...

@chhagedorn thanks for your great suggestion. It did make the code much easier to understand. I've done the refactoring in the latest commit. Please help review. Thanks for your time!

> test/hotspot/jtreg/compiler/c2/TestCondAddDeadBranch.java line 32:
> 
>> 30:  * @run main/othervm -Xcomp -XX:-TieredCompilation -XX:CompileOnly=TestCondAddDeadBranch TestCondAddDeadBranch
>> 31:  * @run main/othervm -Xcomp -XX:-TieredCompilation -XX:CompileOnly=TestCondAddDeadBranch
>> 32:  *                   -XX:+UseCMoveUnconditionally -XX:+UseVectorCmov -XX:MaxVectorSize=32  TestCondAddDeadBranch
> 
> As the cmove flags are C2 specific, we should also add a `@requires vm.compiler2.enabled`. Same for the other test.

Done. Thanks!

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

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