On Thu, 24 Oct 2024 06:31:00 GMT, Emanuel Peter <epe...@openjdk.org> wrote:
>> src/jdk.incubator.vector/share/classes/jdk/incubator/vector/VectorMath.java
>> line 32:
>>
>>> 30: * @since 24
>>> 31: */
>>> 32: public final class VectorMath {
>>
>> I think this class could have been split into a separate RFE, together with
>> its tests. I would prefer that next time.
>
> Also: why did we not add these `Long.minUnsigned` etc? I guess that was
> already discussed?
> Because we can easily also use this with the auto-vectorizer or more
> generally. Saturating and unsigned ops are generally useful I think...
PR is specially targeting explicit vectorization flow, we plan to address
scalar intrinsification and auto-vectorization later, once type system has
exposure to unsigned types.
>> test/jdk/jdk/incubator/vector/templates/Kernel-SaturatingBinary-Masked-op.template
>> line 8:
>>
>>> 6:
>>> 7: for (int ic = 0; ic < INVOC_COUNT; ic++) {
>>> 8: for (int i = 0; i < a.length; i += SPECIES.length()) {
>>
>> I think this does not check if the generated vectors are too long. We had
>> bugs in the past where we should have created say 2-element vectors, but the
>> backend wrongly created 4-element vectors. This is especially an issue with
>> vectors that do direct memory access.
>>
>> With a simple "counting-up" test, you will probably not catch this. It could
>> be good to have a "counting-down" example as well. What do you think?
>
> Also: all of these cases load, and directly store again. Does that not mean
> all tests will probably pick the "..._mem" backend operations? Or do we
> actually end up testing all backend operations with the tests we have here?
To exercise non memory operand pattern we need a vector operation padding layer
after load vector, this will always ensure that selector pick all register
operands flavor of instruction. Since its a generic limitation, do you think we
should float it as a separate PR?
I have create an RFE https://bugs.openjdk.org/browse/JDK-8342959 for
reference. Given that we have moved IR tests out this PR on the grounds of
review complexity, lets not add more code here.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1815000046
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1814998821
