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

[Al K]

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. 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.
>
>
>