On Thu, 24 Oct 2024 06:46:32 GMT, Emanuel Peter <epe...@openjdk.org> wrote:
>> Jatin Bhateja has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Factor out IR tests and Transforms to follow-up PRs.
>
> src/hotspot/cpu/x86/x86.ad line 10790:
>
>> 10788: predicate(is_subword_type(Matcher::vector_element_basic_type(n)) &&
>> 10789: n->is_SaturatingVector() &&
>> !n->as_SaturatingVector()->is_unsigned());
>> 10790: match(Set dst (SaturatingAddV (Binary dst (LoadVector src)) mask));
>
> Do equivalent store operations exist we could also match for?
ISA only supports memory operands as second source operand.
> test/jdk/jdk/incubator/vector/VectorMathTest.java line 70:
>
>> 68: public static short[] INPUT_SS = {Short.MIN_VALUE,
>> (short)(Short.MIN_VALUE + TEN_S), ZERO_S, (short)(Short.MAX_VALUE - TEN_S),
>> Short.MAX_VALUE};
>> 69: public static int[] INPUT_SI = {Integer.MIN_VALUE,
>> (Integer.MIN_VALUE + TEN_I), ZERO_I, Integer.MAX_VALUE - TEN_I,
>> Integer.MAX_VALUE};
>> 70: public static long[] INPUT_SL = {Long.MIN_VALUE, Long.MIN_VALUE
>> + TEN_L, ZERO_L, Long.MAX_VALUE - TEN_L, Long.MAX_VALUE};
>
> Ok, now we have 4 or 5 hand-crafted examples. Is that sufficient? Some random
> values would be nice, then we know that at least eventually we have full
> coverage.
Hand crafter cases contains delimiting and general cases, in short they
sufficiently cover entire value range.
> test/jdk/jdk/incubator/vector/templates/Unit-header.template line 1244:
>
>> 1242: return fill(s * BUFFER_REPS,
>> 1243: i -> ($type$)($Boxtype$.MIN_VALUE + 100));
>> 1244: })
>
> Not sure I see this right. But are we only providing these 4 constants as
> inputs, and all values in the input arrays will be identical? If that is
> true: we should have some random inputs, or at least dependent on `i`.
Most important test points in a saturating operations are the edge conditions
where overflow semantics differs and operation saturates a value than wrapping
it around.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1814998684
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1814999013
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1814998545
