[lworld+vector] RFR: 8311675: [lworld+vector] Max Species support. [v3]
Xiaohong Gong
xgong at openjdk.org
Mon Oct 16 01:58:44 UTC 2023
On Sun, 15 Oct 2023 16:03:50 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:
>> - Patch adds MaxSpecies support for all types of vectors.
>> - New factory methods and VectorPayload classes for various kinds of vector, shuffle and mask payloads.
>> - Disable CDS archiving of VectorPayload classes.
>> - Summary of high level flow :-
>> 1/ Max species payloads encapsulate @multifield annotated field accepting -1 value as bundle size parameter.
>> 2/ For Vector payload bundle size is determined using maximum vector size supported by the target.
>> 3/ For Shuffles and Masks payloads multifield bundle size is a function of maximum vector size and vector lane size.
>> 4/ Based on the dynamic bundle size parser creates a separate FieldInfo structure for each base and synthetic multifield and rest of the flow remains the same.
>>
>> 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.
src/hotspot/cpu/aarch64/vm_version_aarch64.cpp line 663:
> 661: int VM_Version::max_vector_size(BasicType bt) {
> 662: return MaxVectorSize / type2aelembytes(bt);
> 663: }
Please change to use the code in https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/hotspot/cpu/aarch64/aarch64.ad#L2344 like what you have changed in x86 systems, together with the `Matcher::max_vector_size()`. Thanks a lot!
src/hotspot/cpu/x86/vm_version_x86.cpp line 3257:
> 3255: default:
> 3256: assert(false, "Unexpected basic type");
> 3257: }
If the return value is 0, what is the actual number of the multifields in payload? It seems not reasonable for `0`. WDYT?
src/hotspot/share/oops/inlineKlass.cpp line 158:
> 156: if (VectorSupport::is_vector_payload_mf(this) || VectorSupport::is_vector(this)) {
> 157: return false;
> 158: }
Why do you close the array flatten for vectors?
src/hotspot/share/prims/vectorSupport.cpp line 465:
> 463: assert(payload_name, "");
> 464: vmSymbolID sid = vmSymbols::find_sid(payload_name);
> 465: int size = VM_Version::max_vector_size(T_BYTE);
It seems we'd better check whether the return value `size` is `0`. If so, a max with 64-bit size is needed like what we have done in java code. See https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/VectorShape.java#L267
src/jdk.incubator.vector/share/classes/jdk/incubator/vector/AbstractMask.java line 143:
> 141: species.maskType(), species.elementType(), vspecies().laneCount,
> 142: this, species,
> 143: (m, s) -> VectorMask.fromLong(s, m.toLong()).check(s));
Any issues with the original `maskFactory()` ?
-------------
PR Review Comment: https://git.openjdk.org/valhalla/pull/931#discussion_r1360016602
PR Review Comment: https://git.openjdk.org/valhalla/pull/931#discussion_r1360010718
PR Review Comment: https://git.openjdk.org/valhalla/pull/931#discussion_r1360001591
PR Review Comment: https://git.openjdk.org/valhalla/pull/931#discussion_r1360015893
PR Review Comment: https://git.openjdk.org/valhalla/pull/931#discussion_r1360002100
More information about the valhalla-dev
mailing list