Re: RFR: 8376187: [VectorAPI] Define new lane type constants and pass them to intrinsic entries [v4]

2026-02-01 Thread Paul Sandoz
On Sun, 1 Feb 2026 07:36:35 GMT, Jatin Bhateja  wrote:

>> src/jdk.incubator.vector/share/classes/jdk/incubator/vector/AbstractSpecies.java
>>  line 152:
>> 
>>> 150: int laneTypeOrdinal() {
>>> 151: return laneType.ordinal();
>>> 152: }
>> 
>> Is this needed? Won't all concrete sub types override this?
>
> This interface provides access to lane type constant though species, its used 
> for consistency, please have a look at following line and other places around 
> it.
> https://github.com/jatin-bhateja/jdk/blob/ff73dc3d48a9435c4395556c8325fbce7610cba9/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/DoubleVector.java#L3374

Agreed that this method is required, but i was wondering why `AbstractSpecies` 
need to implement it. Ok, i see now you are copying the same pattern as some 
other methods such as `elementType`, so this is a more general issue we should 
not resolve in this PR.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/29481#discussion_r2751614740


Re: RFR: 8376187: [VectorAPI] Define new lane type constants and pass them to intrinsic entries [v4]

2026-01-31 Thread Jatin Bhateja
On Fri, 30 Jan 2026 23:31:29 GMT, Paul Sandoz  wrote:

>> Jatin Bhateja has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Review comments resolutions
>
> src/jdk.incubator.vector/share/classes/jdk/incubator/vector/AbstractSpecies.java
>  line 152:
> 
>> 150: int laneTypeOrdinal() {
>> 151: return laneType.ordinal();
>> 152: }
> 
> Is this needed? Won't all concrete sub types override this?

This interface provides access to lane type constant though species, its used 
for consistency, please have a look at following line and other places around 
it.
https://github.com/jatin-bhateja/jdk/blob/ff73dc3d48a9435c4395556c8325fbce7610cba9/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/DoubleVector.java#L3374

> src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Byte128Vector.java
>  line 60:
> 
>> 58: 
>> 59: static final int LANE_TYPE_ORDINAL = LT_BYTE;
>> 60: 
> 
> You can move this up to `ByteVector` and then reuse it to replace 
> `byte.class`, so it is used consistently.

Done

> src/jdk.incubator.vector/share/classes/jdk/incubator/vector/VectorOperators.java
>  line 821:
> 
>> 819: convert(String name, char kind, Class dom, Class ran, int 
>> opCode, int flags) {
>> 820: int domran = ((LaneType.of(dom).ordinal() << VO_DOM_SHIFT) +
>> 821:   (LaneType.of(ran).ordinal() << VO_RAN_SHIFT));
> 
> As i understand this is still correct because the maximum ordinal value is 
> less than 16 (as was already the case for the basic type).

Correct.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/29481#discussion_r2750675259
PR Review Comment: https://git.openjdk.org/jdk/pull/29481#discussion_r2750675162
PR Review Comment: https://git.openjdk.org/jdk/pull/29481#discussion_r2750675209


Re: RFR: 8376187: [VectorAPI] Define new lane type constants and pass them to intrinsic entries [v4]

2026-01-30 Thread Paul Sandoz
On Fri, 30 Jan 2026 07:35:43 GMT, Jatin Bhateja  wrote:

>> As per [discussions 
>> ](https://github.com/openjdk/jdk/pull/28002#issuecomment-3789507594) on 
>> JDK-8370691 pull request, splitting out portion of PR#28002 into a separate 
>> patch in preparation of Float16 vector API support.
>> 
>> Patch add new lane type constants and pass them to vector intrinsic entry 
>> points.
>> 
>> All existing Vector API jtreg test are passing with the patch.
>> 
>> Kindly review and share your feedback.
>> 
>> Best Regards,
>> Jatin
>
> Jatin Bhateja has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Review comments resolutions

This is looking good. Just one last comment on the location of 
`LANE_TYPE_ORDINAL`.

src/java.base/share/classes/jdk/internal/vm/vector/VectorSupport.java line 152:

> 150: public static final int MODE_BITS_COERCED_LONG_TO_MASK = 1;
> 151: 
> 152: // Lane type codes for vector:

Suggest you change the comment to indicate the values correspond to 
`jdk.incubator.vector.LaneType` ordinals e.g., 
jdk.incubator.vector.LaneType.FLOAT.ordinal() == LT_FLOAT etc.

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/AbstractSpecies.java
 line 152:

> 150: int laneTypeOrdinal() {
> 151: return laneType.ordinal();
> 152: }

Is this needed? Won't all concrete sub types override this?

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Byte128Vector.java 
line 60:

> 58: 
> 59: static final int LANE_TYPE_ORDINAL = LT_BYTE;
> 60: 

You can move this up to `ByteVector` and then reuse it to replace `byte.class`, 
so it is used consistently.

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/LaneType.java line 
270:

> 268: 
> 269: static {
> 270: assert(ofLaneTypeOrdinal(LT_FLOAT) == FLOAT);

Very good!

src/jdk.incubator.vector/share/classes/jdk/incubator/vector/VectorOperators.java
 line 821:

> 819: convert(String name, char kind, Class dom, Class ran, int 
> opCode, int flags) {
> 820: int domran = ((LaneType.of(dom).ordinal() << VO_DOM_SHIFT) +
> 821:   (LaneType.of(ran).ordinal() << VO_RAN_SHIFT));

As i understand this is still correct because the maximum ordinal value is less 
than 16 (as was already the case for the basic type).

-

PR Review: https://git.openjdk.org/jdk/pull/29481#pullrequestreview-3730928806
PR Review Comment: https://git.openjdk.org/jdk/pull/29481#discussion_r2748410039
PR Review Comment: https://git.openjdk.org/jdk/pull/29481#discussion_r2748387527
PR Review Comment: https://git.openjdk.org/jdk/pull/29481#discussion_r2748483577
PR Review Comment: https://git.openjdk.org/jdk/pull/29481#discussion_r2748392412
PR Review Comment: https://git.openjdk.org/jdk/pull/29481#discussion_r2748427970


Re: RFR: 8376187: [VectorAPI] Define new lane type constants and pass them to intrinsic entries [v4]

2026-01-29 Thread Jatin Bhateja
> As per [discussions 
> ](https://github.com/openjdk/jdk/pull/28002#issuecomment-3789507594) on 
> JDK-8370691 pull request, splitting out portion of PR#28002 into a separate 
> patch in preparation of Float16 vector API support.
> 
> Patch add new lane type constants and pass them to vector intrinsic entry 
> points.
> 
> All existing Vector API jtreg test are passing with the patch.
> 
> Kindly review and share your feedback.
> 
> Best Regards,
> Jatin

Jatin Bhateja has updated the pull request incrementally with one additional 
commit since the last revision:

  Review comments resolutions

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/29481/files
  - new: https://git.openjdk.org/jdk/pull/29481/files/d81035fd..ff73dc3d

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=29481&range=03
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=29481&range=02-03

  Stats: 858 lines in 48 files changed: 290 ins; 13 del; 555 mod
  Patch: https://git.openjdk.org/jdk/pull/29481.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/29481/head:pull/29481

PR: https://git.openjdk.org/jdk/pull/29481