[vector] Intrinsics for fromByteArray, fromByteBuffer, intoByteArray, intoByteBuffer

[Al K]

Right, concern is mainly around the type checking/casting and correct
use, I haven't dealt with any of this before so it's new to me. To clarify
I'm accessing the base + address correctly, as you suggested, using
var handles as an example, f.e. store,

    @Override
    @ForceInline
    public void intoByteBuffer(ByteBuffer bb, int ix) {
        if (!bb.isReadOnly() && bb.order() == ByteOrder.nativeOrder()) {
            int num_bytes = bitSize() / Byte.SIZE;
            int ax = VectorIntrinsics.checkIndex(ix, bb.limit(), num_bytes);
            VectorIntrinsics.store($vectortype$.class, $type$.class, LENGTH,
                                   bb, VectorIntrinsics.UNSAFE.getObject(bb,
BYTE_BUFFER_HB),
                                   VectorIntrinsics.UNSAFE.getLong(bb,
BUFFER_ADDRESS) + ax,
                                   this, (base, off) ->
super.intoByteBuffer(bb, ax));
        } else {
            super.intoByteBuffer(bb, ix);
        }
    }

But past that, I'm unsure what inline_vector_mem_operation should look like.
The diff I pasted above seems to work, and generates okay assembly, though
I haven't tested it much.

On Tue, May 8, 2018 at 5:43 PM, Paul Sandoz <paul.sandoz at oracle.com> wrote:

>
>
> On May 8, 2018, at 3:31 PM, Al K <someusername3 at gmail.com> wrote:
>
> Got it, thanks. Mainly wondering what the changes for Paul's suggestion
> would
> look like, I don't have a good idea as to how the on-/off-heap would work
> for
> a byte buffer.
>
>
> It’s easy to uniformly extract out the base + address from a byte buffer.
> That’s what VarHandles do (using unsafe accesses), and also vectorized
> buffer comparison support (using methods/fields because its in the same
> package, plus there are some other constraints for uniform access across
> all buffers, not just byte buffers) e.g. see code in java.nio.
> BufferMismatch:
>
> static int mismatch(ByteBuffer a, int aOff, ByteBuffer b, int bOff, int length) {
>     int i = 0;
>     if (length > 7) {
>         i = ArraysSupport.vectorizedMismatch(
>                 a.base(), a.address + aOff,
>                 b.base(), b.address + bOff,
>                 length,
>                 ArraysSupport.LOG2_ARRAY_BYTE_INDEX_SCALE);
>         if (i >= 0) return i;
>         i = length - ~i;
>     }
>     for (; i < length; i++) {
>         if (a.get(aOff + i) != b.get(bOff + i))
>             return i;
>     }
>     return -1;
> }
>
> Paul.
>
> Something like below is what I have, though I don't think heap byte
> buffers would result in using_byte_array being true or if the usage is
> correct at all,
>
> -  Node* arr = argument(3);
> -  Node* idx = argument(4);
> -
> -  const TypeAryPtr* arr_type = gvn().type(arr)->isa_aryptr();
> -  if (arr_type == NULL) {
> -    return false; // should be an array
> -  }
> +  Node* container = argument(3);
> +  Node* base = argument(4);
> +  Node* offset = ConvL2X(argument(5));
> +  Node* addr = make_unsafe_address(base, offset, elem_bt, true);
> +  const TypePtr *addr_type = gvn().type(addr)->isa_ptr();
> +
> +  const TypeAryPtr* arr_type = addr_type->isa_aryptr();
>
>    // Now handle special case where load/store happens from/to byte array
> but element type is not byte.
> -  bool using_byte_array = arr_type->elem()->array_element_basic_type()
> == T_BYTE && elem_bt != T_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 && elem_bt != arr_type->elem()->array_element_basic_type())
> {
> +  if (!using_byte_array && arr_type != NULL && elem_bt !=
> arr_type->elem()->array_element_basic_type()) {
>      return false;
>    }
>
> -  Node* adr = array_element_address(arr, idx, using_byte_array ? T_BYTE :
> elem_bt);
> -  const TypePtr* adr_type = adr->bottom_type()->is_ptr();
> +  /*Node* adr = array_element_address(base, idx, using_byte_array ?
> T_BYTE : elem_bt);
> +  const TypePtr* adr_type = adr->bottom_type()->is_ptr();*/
>
>    ciKlass* vbox_klass = vector_klass->const_oop()->as_
> instance()->java_lang_Class_klass();
>    const TypeInstPtr* vbox_type = TypeInstPtr::make_exact(TypePtr::NotNull,
> vbox_klass);
>
>    if (is_store) {
> -    Node* val = unbox_vector(argument(5), vbox_type, elem_bt, num_elem);
> +    Node* val = unbox_vector(argument(6), vbox_type, elem_bt, num_elem);
>      if (val == NULL) {
>        return false; // operand unboxing failed
>      }
> @@ -7440,19 +7440,19 @@
>        val = gvn().transform(new VectorReinterpretNode(val,
> val->bottom_type()->is_vect(), to_vect_type));
>      }
>
> -    Node* vstore = gvn().transform(StoreVectorNode::make(0, control(),
> memory(adr), adr, adr_type, val, store_num_elem));
> -    set_memory(vstore, adr_type);
> +    Node* vstore = gvn().transform(StoreVectorNode::make(0, control(),
> memory(addr), addr, addr_type, val, store_num_elem));
> +    set_memory(vstore, addr_type);
>      set_vector_result(vstore, false);
>    } else {
>      // When using byte array, we need to load as byte then reinterpret
> the value. Otherwise, do a simple vector load.
>      Node* vload = NULL;
>      if (using_byte_array) {
>        int load_num_elem = num_elem * type2aelembytes(elem_bt);
> -      vload = gvn().transform(LoadVectorNode::make(0, control(),
> memory(adr), adr, adr_type, load_num_elem, T_BYTE));
> +      vload = gvn().transform(LoadVectorNode::make(0, control(),
> memory(addr), addr, addr_type, load_num_elem, T_BYTE));
>        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(adr), adr, adr_type, num_elem, elem_bt));
> +      vload = gvn().transform(LoadVectorNode::make(0, control(),
> memory(addr), addr, addr_type, num_elem, elem_bt));
>      }
>
> On Mon, May 7, 2018 at 3:36 PM, Lupusoru, Razvan A <
> razvan.a.lupusoru at intel.com> wrote:
>
>> Hi Al,
>>
>>
>>
>> I am not quite certain of the answer to your question but I’d like to
>> explain why that node is used in implementation.
>>
>>
>>
>> Basically, VectorReinterpretNode is intended to allow “type conversion”
>> of vectors while keeping contents unmodified. It also allows for compiler
>> to keep track of appropriate types. It does not have anything to do with
>> whether an access is on heap or off heap.
>>
>>
>>
>> So let us say we have a node of TypeVect “#vectory[8]:{float}”. Since
>> when using ByteBuffer and byte arrays we explicitly check that we have a
>> byte array, we mark that the “address” of the array is of type
>> “@byte[int:>=0]:exact+any *”. Thus the VectorReinterpret node is used as a
>> transition node to convert from “#vectory[8]:{float}” to type
>> “#vectory[32]:{byte}”. Thus now we have checkable strong typing in place
>> where we do not mix and allow store to memory that does not match the type
>> of what we are storing.
>>
>>
>>
>> I am not very familiar with the type of unsafe memory and whether
>> compiler will complain if your vector type is not consistent with your
>> memory type. My guess is that there is special marking used for unsafe
>> accesses - and thus it would seem to me that VectorReinterpret node is not
>> necessarily needed. That said, the node itself does not add any overheads
>> (as long as input and output type have same size), and thus it’s not a
>> problem to keep it.
>>
>>
>>
>> Hopefully my answer is somewhat helpful in answering your question.
>>
>>
>>
>> Thanks,
>>
>> Razvan
>>
>>
>>
>> *From:* Al K [mailto:someusername3 at gmail.com]
>> *Sent:* Monday, May 07, 2018 12:51 PM
>> *To:* Lupusoru, Razvan A <razvan.a.lupusoru at intel.com>
>> *Cc:* Paul Sandoz <paul.sandoz at oracle.com>; John Rose <
>> john.r.rose at oracle.com>; panama-dev at openjdk.java.net
>>
>> *Subject:* Re: [vector] Intrinsics for fromByteArray, fromByteBuffer,
>> intoByteArray, intoByteBuffer
>>
>>
>>
>> Is VectorReinterpretNode required when loading from/storing to off-heap
>> buffer? Wondering
>>
>> if it's sufficient enough to just invoke make_unsafe_address() with the
>> base and offset, and
>>
>> continue on, or if there's more complications associated with it.
>>
>>
>>
>> On Fri, May 4, 2018 at 1:54 PM, Lupusoru, Razvan A <
>> razvan.a.lupusoru at intel.com> wrote:
>>
>> Paul and John,
>>
>> I just merged the patch with the following changes which stemmed from
>> your discussion:
>>
>> -          No longer allow writing to a read only buffer
>>
>> -          No longer intrinsify if bytebuffer endianness does not match
>> native order
>>
>> o   There is no silent conversion taking place.
>>
>> Thanks for your reviews in finding the various issues. I agree that a
>> worthy improvement would be to allow writing to buffers that do not have
>> backing array by using the Unsafe mechanism you recommended.
>>
>> Thanks!
>>
>> --Razvan
>>
>> From: Paul Sandoz [mailto:paul.sandoz at oracle.com]
>> Sent: Thursday, May 03, 2018 8:52 AM
>> To: John Rose <john.r.rose at oracle.com>
>> Cc: Lupusoru, Razvan A <razvan.a.lupusoru at intel.com>;
>> panama-dev at openjdk.java.net
>> Subject: Re: [vector] Intrinsics for fromByteArray, fromByteBuffer,
>> intoByteArray, intoByteBuffer
>>
>>
>>
>> On May 3, 2018, at 8:48 AM, John Rose <john.r.rose at oracle.com<mailto:
>> john.r.rose at oracle.com>> wrote:
>>
>> On May 3, 2018, at 8:40 AM, Paul Sandoz <paul.sandoz at oracle.com<mailto:
>> paul.sandoz at oracle.com>> wrote:
>>
>> stick to native endianness for the moment to keep things simpler initially
>>
>> I suggest throwing an exception if the buffer is in the wrong endianness.
>> We have regretted silently converting to native on other occasions.
>>
>> A good point, it will remind us to fix it and revisit the specification.
>> I will add placeholder in the load/store tests.
>>
>> Paul.
>>
>>
>>
>
>
>