[VectorAPI]maskFromArray Intrinsic failure
Vladimir Ivanov
vladimir.x.ivanov at oracle.com
Wed Jan 16 20:40:40 UTC 2019
Looks good.
Best regards,
Vladimir Ivanov
On 16/01/2019 04:46, Wang Zhuo(Zhuoren) wrote:
> Webrev files have been uploaded to
> http://cr.openjdk.java.net/~luchsh/mask_fix_zhuoren/. Please review.
>
> Regards,
> Zhuoren
>
> ------------------------------------------------------------------
> From:Vladimir Ivanov <vladimir.x.ivanov at oracle.com>
> Sent At:2019 Jan. 16 (Wed.) 02:46
> To:Sandler <zhuoren.wz at alibaba-inc.com>; panama-dev
> <panama-dev at openjdk.java.net>
> Cc:li sanhong <sanhong.lsh at alibaba-inc.com>; Kingsum Chow
> <kingsum.kc at alibaba-inc.com>
> Subject:Re: [VectorAPI]maskFromArray Intrinsic failure
>
> Good catch!
>
> src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Int128Vector.java:
> > - bits, (((long) ix) <<
> ARRAY_SHIFT) + Unsafe.ARRAY_BOOLEAN_BASE_OFFSET,
> > + bits, ((long) ix) +
> Unsafe.ARRAY_BOOLEAN_BASE_OFFSET,
>
> It's better to replace ARRAY_SHIFT with Unsafe.ARRAY_BOOLEAN_INDEX_SCALE.
>
>
> src/hotspot/share/opto/library_call.cpp:
>
> > + if (!arch_supports_vector(Op_LoadVector, num_elem, T_BOOLEAN,
> VecMaskUseLoad)) {
>
> Why do you specify VecMaskUseLoad and not VecMaskNotUsed (as in other
> cases)?
>
> Otherwise, looks good for now. Please, send complete webrev for review.
>
> As a followup, it would be nice to refactor/cleanup Mask-related code on
>
> JDK-JVM boundary (VectorIntrinsics + library_call.cpp). For example, I
> don't see a reason to pass int.class (and not boolean.class) as the
> element type for mask-related operations.
>
> Best regards,
> Vladimir Ivanov
>
> On 15/01/2019 08:37, Wang Zhuo(Zhuoren) wrote:
> > Hi,
> > I found intrinsics for maskFromArray always failed. Here is a patch to fix this issue.
> > Benchmarks containing heavy maskFromArray use can benifit from this patch. such as allTrue/anyTrue.
> >
> > This patch consists of two parts. In the JDK part, since boolean is only one byte, it is not correct to perform left shift ARRAY_SHIFT for array index. I listed only one file in the patch to avoid a long letter, but this modification should be applied to all types including the template file.
> > return VectorIntrinsics.load(Int128Mask.class, int.class, LENGTH,
> > - bits, (((long) ix) << ARRAY_SHIFT) + Unsafe.ARRAY_BOOLEAN_BASE_OFFSET,
> > + bits, ((long) ix) + Unsafe.ARRAY_BOOLEAN_BASE_OFFSET,
> > bits, ix,
> > Another part is in hotspot, please check the below patch
> >
> > diff -r 890e996dfd1a src/hotspot/share/opto/library_call.cpp
> > --- a/src/hotspot/share/opto/library_call.cpp Mon Jan 07 10:54:21 2019 +0800
> > +++ b/src/hotspot/share/opto/library_call.cpp Tue Jan 15 23:45:40 2019 +0800
> > @@ -7069,13 +7069,13 @@
> >
> > // Now handle special case where load/store happens from/to byte array but element type is not byte.
> > bool using_byte_array = arr_type != NULL && arr_type->elem()->array_element_basic_type() == T_BYTE && elem_bt != T_BYTE;
> > -
> > - // It must be the case that if it is not special byte array case, there is consistency between
> > - // array and vector element types.
> > - if (!using_byte_array && arr_type != NULL && elem_bt != arr_type->elem()->array_element_basic_type()) {
> > + // Handle loading masks.
> > + ciKlass* vbox_klass = vector_klass->const_oop()->as_instance()->java_lang_Class_klass();
> > + bool is_mask = vbox_klass->is_vectormask();
> > + // If there is no consistency between array and vector element types, it must be special byte array case or loading masks
> > + if (!using_byte_array && arr_type != NULL && elem_bt != arr_type->elem()->array_element_basic_type() && !is_mask) {
> > return false;
> > }
> > -
> > // Since we are using byte array, we need to double check that the byte operations are supported by backend.
> > if (using_byte_array) {
> > int byte_num_elem = num_elem * type2aelembytes(elem_bt);
> > @@ -7083,8 +7083,12 @@
> > return false; // not supported
> > }
> > }
> > -
> > - ciKlass* vbox_klass = vector_klass->const_oop()->as_instance()->java_lang_Class_klass();
> > + if (is_mask) {
> > + if (!arch_supports_vector(Op_LoadVector, num_elem, T_BOOLEAN, VecMaskUseLoad)) {
> > + return false; // not supported
> > + }
> > + }
> > +
> > const TypeInstPtr* vbox_type = TypeInstPtr::make_exact(TypePtr::NotNull, vbox_klass);
> >
> > if (is_store) {
> > @@ -7114,9 +7118,15 @@
> > const TypeVect* to_vect_type = TypeVect::make(elem_bt, num_elem);
> > vload = gvn().transform(new VectorReinterpretNode(vload, vload->bottom_type()->is_vect(), to_vect_type));
> > } else {
> > - vload = gvn().transform(LoadVectorNode::make(0, control(), memory(addr), addr, addr_type, num_elem, elem_bt));
> > - }
> > -
> > + // Special handle for masks
> > + if (is_mask) {
> > + vload = gvn().transform(LoadVectorNode::make(0, control(), memory(addr), addr, addr_type, num_elem, T_BOOLEAN));
> > + const TypeVect* to_vect_type = TypeVect::make(elem_bt, num_elem);
> > + vload = gvn().transform(new VectorLoadMaskNode(vload, to_vect_type));
> > + } else {
> > + vload = gvn().transform(LoadVectorNode::make(0, control(), memory(addr), addr, addr_type, num_elem, elem_bt));
> > + }
> > + }
> > Node* box = box_vector(vload, vbox_type, elem_bt, num_elem);
> > set_vector_result(box);
> > }
> > --- a/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Int128Vector.java Mon Jan 07 10:54:21 2019 +0800
> > +++ b/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/Int128Vector.java Tue Jan 15 23:45:40 2019 +0800
> > @@ -1338,7 +1338,7 @@
> > Objects.requireNonNull(bits);
> > ix = VectorIntrinsics.checkIndex(ix, bits.length, LENGTH);
> > return VectorIntrinsics.load(Int128Mask.class, int.class, LENGTH,
> > - bits, (((long) ix) << ARRAY_SHIFT) + Unsafe.ARRAY_BOOLEAN_BASE_OFFSET,
> > + bits, ((long) ix) + Unsafe.ARRAY_BOOLEAN_BASE_OFFSET,
> > bits, ix,
> > (c, idx) -> opm(n -> c[idx + n]));
> >
> > Regards,
> > Zhuoren
> >
>
More information about the panama-dev
mailing list