RFR: 8342677: Add IR validation tests for newly added saturated vector add / sub operations

[Emanuel Peter]

On Mon, 21 Oct 2024 10:52:01 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

> This is a follow up PR to https://github.com/openjdk/jdk/pull/20507
> It adds IR validation tests for newly added saturated vector add / sub operations.

Thanks for adding these tests! I agree, it would be nice to get them in for JDK24. If we miss RDP1 we could even consider backporting it.

test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java line 257:

> 255:     }
> 256: 
> 257:     public static final String SADD_VB = VECTOR_PREFIX + "SADD_VB" + POSTFIX;

Suggestion:

    public static final String SATURATING_ADD_VB = VECTOR_PREFIX + "SATURATING_ADD_VB" + POSTFIX;

I would prefer if this was written out.

test/hotspot/jtreg/compiler/vectorapi/VectorSaturatedOperationsTest.java line 26:

> 24: /**
> 25: * @test
> 26: * @bug 8342677

Can you add the bug number of the original feature? Because that is really what we are testing for here.

test/hotspot/jtreg/compiler/vectorapi/VectorSaturatedOperationsTest.java line 97:

> 95:                     short_in2[i] = (short)-i;
> 96:                     byte_in1[i]  = Byte.MIN_VALUE;
> 97:                     byte_in2[i]  = (byte)-i;

Are these values sufficient?

Example with `SADD_VL`:
if the itt elements are `max_value` and `min_value` -> no overflow -> `-1`
if the ith elements are `-i` and `i` -> no overflow -> `0`

So it seems we are actually not testing the saturation here, am I correct?

test/hotspot/jtreg/compiler/vectorapi/VectorSaturatedOperationsTest.java line 200:

> 198: 
> 199:     @Test
> 200:     @IR(counts = {IRNode.SADD_VB, " >0 " , "unsigned_vector_node", " >0 "}, phase = {CompilePhase.BEFORE_MATCHING}, applyIfCPUFeature = {"avx", "true"})

Suggestion:

    @IR(counts = {IRNode.SADD_VB, " >0 " ,
                  "unsigned_vector_node", " >0 "},
        phase = {CompilePhase.BEFORE_MATCHING},
        applyIfCPUFeature = {"avx", "true"})

I find this generally more readable, than a long line.
What is the `unsigned_vector_node` from, i.e. what is the whole line it maches on?

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

Changes requested by epeter (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21603#pullrequestreview-2469177514
PR Review Comment: https://git.openjdk.org/jdk/pull/21603#discussion_r1863014176
PR Review Comment: https://git.openjdk.org/jdk/pull/21603#discussion_r1863014676
PR Review Comment: https://git.openjdk.org/jdk/pull/21603#discussion_r1863026590
PR Review Comment: https://git.openjdk.org/jdk/pull/21603#discussion_r1863022494