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

[Christian Hagedorn]

On Mon, 10 Oct 2022 06:12:11 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.

Otherwise, the fix looks reasonable to me!

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

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

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