Re: [VectorAPI]maskFromArray Intrinsic failure
Wang Zhuo(Zhuoren)
zhuoren.wz at alibaba-inc.com
Wed Jan 16 12:46:26 UTC 2019
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