RFR: 8309893: Integrate ReplicateB/S/I/L/F/D nodes to Replicate node [v2]

[Emanuel Peter]

On Tue, 24 Oct 2023 06:39:51 GMT, Eric Liu <eliu at openjdk.org> wrote:

>> This patch creates ReplicateNode to replace ReplicateB/S/I/L/F/DNode, like other vector nodes introduced recently, e.g., PopulateIndexNode and ReverseVNode, etc. This refers from:
>> https://mail.openjdk.org/pipermail/panama-dev/2020-April/008484.html
>> 
>> After merging these nodes, code will be easier to maintain. E.g., matching rules can be simplified.
>> 
>> Besides AArch64, this patch tries to keep other ad files as the same before, only supplies some necessary predicate. E.g., for matching rules using ReplicateB before, they are now matching Replicate with a new predicate "Matcher::vector_element_basic_type(n) == T_BYTE". This would be easy for review and lower risks.
>> 
>> [TEST]
>> x86:     Tested with option "-XX:UseAVX=0/1/2/3".
>> AArch64: Tested on SVE machine and Neon machine.
>> 
>> Full jtreg passed without new issue.
>
> Eric Liu has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
> 
>  - small fix
>    
>    Change-Id: I121a8c1ad75e88c0af41f2c1ef00289a3d61f400
>  - Merge jdk:master
>    
>    Change-Id: I4df10ba7290e9af02deeac5a931234a07be7c207
>  - 8309893: Integrate ReplicateB/S/I/L/F/D nodes to Replicate node
>    
>    This patch creates ReplicateNode to replace ReplicateB/S/I/L/F/DNode,
>    like other vector nodes introduced recently, e.g., PopulateIndexNode and
>    ReverseVNode, etc. This refers from:
>    https://mail.openjdk.org/pipermail/panama-dev/2020-April/008484.html
>    
>    After merging these nodes, code will be easier to maintain. E.g.,
>    matching rules can be simplified.
>    
>    Besides AArch64, this patch tries to keep other ad files as the same
>    before, only supplies some necessary predicate. E.g., for matching rules
>    using ReplicateB before, they are now matching Replicate with a new
>    predicate "Matcher::vector_element_basic_type(n) == T_BYTE". This would
>    be easy for review and lower risks.
>    
>    [TEST]
>    x86:     Tested with option "-XX:UseAVX=0/1/2/3".
>    AArch64: Tested on SVE machine and Neon machine.
>    
>    Full jtreg passed without new issue.
>    
>    Change-Id: Icb16084e3909177ab302f0ba33c2216c860a5da6

Generally looks really good, thanks for the work!
I left a few small comments.

I also did not really review:
`src/hotspot/cpu/aarch64/aarch64_vector_ad.m4`
I don't understand those parts enough to review it.

src/hotspot/share/opto/matcher.cpp line 2800:

> 2798:   BasicType bt = vector_element_basic_type(n);
> 2799:   assert(bt != T_CHAR, "char is not allowed in vector");
> 2800:   return is_subword_type(bt) || bt == T_INT;

I suggest you create a `is_non_long_integral_vector` in `src/hotspot/share/utilities/globalDefinitions.hpp`, next to `is_integral_type`.

src/hotspot/share/opto/vectornode.cpp line 609:

> 607: // Check if input is loop invariant vector.
> 608: bool VectorNode::is_invariant_vector(Node* n) {
> 609:   // Only Replicate vector nodes are loop invariant for now.

Random discovery: does this not sound fishy? Can Replicate nodes never be used in a loop variant way? For example with the VectorAPI?

test/hotspot/jtreg/compiler/vectorapi/VectorCompareWithImmTest.java line 130:

> 128: 
> 129:     @Test
> 130:     @IR(counts = { IRNode.VMASK_CMP_IMM_I_SVE, ">= 1" })

Is this ok, or rather a downgrade?
Or maybe you should remove the `I` for int in the name?

test/hotspot/jtreg/compiler/vectorization/runner/ArrayInvariantFillTest.java line 75:

> 73:     @IR(applyIfCPUFeatureOr = {"asimd", "true", "sse2", "true"},
> 74:         applyIf = {"OptimizeFill", "false"},
> 75:         counts = {IRNode.REPLICATE, ">0"})

This may be a good time to try and convert `IRNode.REPLICATE` to a `vectorNode`. That would ensure that we still can use the type of the replicate in this test here.

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

PR Review: https://git.openjdk.org/jdk/pull/14830#pullrequestreview-1696804733
PR Review Comment: https://git.openjdk.org/jdk/pull/14830#discussion_r1371433015
PR Review Comment: https://git.openjdk.org/jdk/pull/14830#discussion_r1371446406
PR Review Comment: https://git.openjdk.org/jdk/pull/14830#discussion_r1371451905
PR Review Comment: https://git.openjdk.org/jdk/pull/14830#discussion_r1371461387