On Tue, 3 Sep 2024 13:09:13 GMT, Emanuel Peter <epe...@openjdk.org> wrote:
> You did in fact add `java/lang` methods. I think you need to add tests for
> all of those. As well. That's going to be even more code to review.
Hi @eme64 , As Paul suggested in offline mail chain, lets restrict the changes
with this patch to only VectorAPI. Going forward we may need to add special
Unsigned value classes wrapping around equivalent sized integers. For the time
being moving all the helper APIs int VectorMathUtils.java, these automatically
gets exercised by the fallback implementation and we already have tests for
next APIs.
> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 914:
>
>> 912: case T_SHORT: vpminuw(dst, src1, src2, vlen_enc); break;
>> 913: case T_INT: vpminud(dst, src1, src2, vlen_enc); break;
>> 914: case T_LONG: evpminuq(dst, k0, src1, src2, false, vlen_enc);
>> break;
>
> Can you explain to me what the `k0` is and where it comes from?
k0 is an implicit mask register which signifies all true mask. Its not
allocatable. Long min / max instructions are only available on AVX512 targets.
> src/hotspot/share/opto/addnode.hpp line 194:
>
>> 192: class SaturatingAddINode : public Node {
>> 193: public:
>> 194: SaturatingAddINode(Node* in1, Node* in2) : Node(in1,in2) {}
>
> Suggestion:
>
> SaturatingAddINode(Node* in1, Node* in2) : Node(in1, in2) {}
>
> In other places below as well.
Not applicable now.
> src/hotspot/share/opto/addnode.hpp line 198:
>
>> 196: virtual const Type* bottom_type() const { return TypeInt::INT; }
>> 197: virtual uint ideal_reg() const { return Op_RegI; }
>> 198: };
>
> Are these not supposed to inherit from the `AddNode`, and then override the
> corresponding methods? Or are you making them separate for a good reason?
As per offline discussion with Paul, we are planning to restrict this patch to
only Vector API, please refer to my earlier comments,
https://github.com/openjdk/jdk/pull/20507#discussion_r1718044262
To reduce the noise I am keeping only required Vector IR nodes and planning to
support scalar saturated operations in subsequent patch.
> src/hotspot/share/opto/addnode.hpp line 462:
>
>> 460:
>> //------------------------------UMaxINode---------------------------------------
>> 461: // Maximum of 2 unsigned integers.
>> 462: class UMaxLNode : public Node {
>
> Here you comment it with `UMaxINode`, but below it is the `UMaxLNode`. The
> `-------xyz------` comments are really useless. But the semantics description
> is useful (though you again say integer instead of long here...).
Not applicable now.
> src/hotspot/share/opto/vectornode.hpp line 634:
>
>> 632: virtual int Opcode() const;
>> 633: };
>> 634:
>
> This could also be a separate PR. Or are they somehow inseparable from the
> "saturation" changes?
Not applicable now.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/20507#issuecomment-2330830123
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1744971176
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1744970961
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1744971087
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1744971023
PR Review Comment: https://git.openjdk.org/jdk/pull/20507#discussion_r1744970833
