RFR: 8264954: unified handling for VectorMask object re-materialization during de-optimization

[Vladimir Ivanov]

On Fri, 9 Apr 2021 07:25:11 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

> Following flow describes object reconstruction for de-optimization:-
> 1)	PhaseVector::scalarize_vbox_node() creates SafePointScalarObjectNode to captures the box type information, also it connects to node holding the boxed value.
> 2)	During code emit phase (PhaseOutput) C2 process above information to dumps ObjectValue holding the box information and LocationValue to holding the value information into ScopeDescriptor corresponding to Safepoint PC.
> 3)	De-optimization blobs dump the value held in registers to the stack locations using RegisterSave::save_live_registers() and a mapping b/w register and its stack location is added to RegisterMap.
> 4)	During de-optimization, compiled frame objects are re-allocated using identity information held in ObjectValue and their fields are initialized using values held in the stack locations accessed through register-stack mappings. 
> 
> By inserting a VectorStoreMaskNode before stitching the mask holding node to Safepoint we make sure that value held in opmask/vector register is transferred to a byte vector. Thus rest of the flow works as it is, stack location will hold the value in the form of a byte array irrespective of the box shape.
> 
> tier1-tier3 regressions are clean with UseAVX=2/3.

Very nice! I like your idea to unify mask representation. It simplifies implementation a lot.
But it also means there'll be 2 representations kept alive for masks with debug usages and burns 2 registers (speaking of x86, either 1 predicate + 1 vector registers on AVX512 or 2 vector regsiters on pre-AVX512). Do you see any problems with that? (It can be improved later if it turns out to be a problem in practice.)

src/hotspot/share/opto/vector.cpp line 277:

> 275:         const TypeVect* vt = vec_value->bottom_type()->is_vect();
> 276:         BasicType bt = vt->element_basic_type();
> 277:         vec_value = gvn.transform(VectorStoreMaskNode::make(gvn, vec_value, bt, vt->length()));

`VectorStoreMaskNode::Identity()` already handles `VectorStoreMask (VectorLoadMask bv) elem_size ==> bv` transformation, doesn't it? So, you can just unconditionally instantiate `VectorStoreMaskNode` and let IGVN handle it.

Also, an observation on naming: `VectorLoadMask` and `VectorStoreMask` names are misleading. On the surface, they look like memory nodes, but in fact, are cast nodes (`Vector <-> Mask`). Please, file an RFE to address that eventually.

src/hotspot/share/prims/vectorSupport.cpp line 91:

> 89: void VectorSupport::init_payload_element(typeArrayOop arr, bool is_mask, BasicType elem_bt, int index, address addr) {
> 90:   if (is_mask) {
> 91:     // Masks require special handling: when boxed they are packed and stored in boolean

The comment is still valid. Please, keep it and update with latest details about mask support.

src/hotspot/share/prims/vectorSupport.cpp line 97:

> 95:       case T_LONG:
> 96:       case T_FLOAT:
> 97:       case T_DOUBLE: arr->bool_at_put(index,  (*(jbyte*)addr) != 0); break;

No switch needed anymore. Just leave the  `arr->bool_at_put(index,  (*(jbyte*)addr) != 0)`.
As an option, consider adding an assert (`assert(is_java_type(bt) && bt != T_BOOLEAN)`.

src/hotspot/share/prims/vectorSupport.cpp line 124:

> 122:   // byte array for masks present in both predicated register
> 123:   // or vector registers.
> 124:   int elem_size = is_mask ? 1 : type2aelembytes(elem_bt);

If `elem_bt == T_BOOLEAN` for masks, you can get rid of `is_mask` usages in `VectorSupport::allocate_vector_payload_helper()` and just introduce a special case in `VectorSupport::klass2bt` (as already done for `VectorShuffle`s).

-------------

PR: https://git.openjdk.java.net/jdk/pull/3408