[vector] Intrinsics for fromByteArray, fromByteBuffer, intoByteArray, intoByteBuffer
Paul Sandoz
paul.sandoz at oracle.com
Tue May 8 23:48:45 UTC 2018
> On May 8, 2018, at 3:56 PM, Al K <someusername3 at gmail.com> wrote:
>
> 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);
> }
> }
>
Great!
> 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.
I am not an except here but what you have done seem like in the right direction. The element type is tracked in the unsafe address node.
I am unsure about the calculation of arr_type, does it works for ByteBuffer? we may need to be consistent in those two cases, if the using_byte_array logic is still required (which may more accurately means we are scribbling on a memory region).
Paul.
>
> 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.
>>
>>
>>
>>
>
>
More information about the panama-dev
mailing list