[lworld+vector] RFR: 8314628: [lworld+vector] validation regression fixes and cleanups.

[Xiaohong Gong]

On Mon, 21 Aug 2023 07:36:19 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

> Patch fixes following problem seen during validation:-
> 
> **CDS:-**
> - Problem while restoring value object mirrors. These are needed to perform class level comparisons.
> 
> **VM:-**
> - Remove payload offset computation logic during VM initialization. It was earlier used during object re-materialization.
> 
> **ci Model:-**
> - Add a generic routine to handle synthetic ciField population for both static and non-static multifield. This enables returning fully populated ciMultiField structure to compilers during parsing.
> 
> **C1 compiler:-**
> - Incorrect address displacement computation in HIR while updating multifield bundle using "withfield" bytecode in VectorPayalod constructor. This was causing  failures in Double128Mask tests.
> 
> **C2 compiler:-**
> 1.  Allow vector IR creation under strict check for multifield in default_value creation routine. This was incorrectly creating vector IR for non-vector API tests for Double/Long types.
> 2. Disable inline expansion of Unsafe.finishPrivateBuffer.  This is fixing the incorrectness problems seen with masked operations loop e.g.
> 
>>     vt = newPayload();
>>     vt = Unsafe.makePrivateBuffer(vt);
>>     for (int i = 0; i < Double128.length; i++) {
>>          if (mask[i]) {
>>               Unsafe.putDouble(vt, apply(src[i]));
>>          }
>>     }
> 
>    Unsafe.putDouble() only modifies the memory state by storing a new value in payload buffer but does not update any local variable in the JVM state corresponding to latch block. Thus no inductive Phi node is inserted in the loop, it also re-materializes InlineTypeNode from modified memory state. This causes incorrectness if second mask value is false as entire payload is overwritten with default value and earlier lane updates are over-written. In order to be safe, disabling intrinsification of finishPrivateBuffer if incoming argument node is of VectorPayload* type, so that it always receive updated buffer argument.
> 
> 3. With non-incremental in-lining (-XX:-IncrementalInline) flow user defined routines returning a vector object is incorrectly returning uninitialized VBA connected to oop input of InlineType IR. Defer returning VectorBoxAllocation node, they are expanded and initialized during box expansion and replaces all uses of box.
> 
> 4. Currently we try to sharpen the object type to a narrower type to optimize object type comparison against a class constant. This is achieved by inserting a CheckCastPP node which casts the object type to a higher speculative type, if cast type is an inlinetyp...

src/hotspot/share/c1/c1_GraphBuilder.cpp line 2207:

> 2205:             StoreField* store = new StoreField(new_instance, offset + sec_offset, temp, replacement, false, state_before, false);
> 2206:             append(store);
> 2207:           }

Do you have a further plan to refactor the multifields parser in ci stage so that we can remove these similar handling for multifields in many places?

src/hotspot/share/classfile/javaClasses.cpp line 4989:

> 4987:   o->obj_field_put(_payload_offset, val);
> 4988: }
> 4989: 

Can we also clean-up the classfile symbols in https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/hotspot/share/classfile/vmClassMacros.hpp#L197 and https://github.com/openjdk/valhalla/blob/lworld%2Bvector/src/hotspot/share/classfile/vmSymbols.hpp#L97 ?

src/hotspot/share/opto/inlinetypenode.cpp line 513:

> 511:       bool mismatched = (decorators & C2_MISMATCHED) != 0;
> 512:       ciField* field = holder->get_field_by_offset(offset, false);
> 513:       if (base->is_Con() && !is_array && !mismatched && !field->is_multifield_base() && ft->bundle_size() == 1) {

Could we remove the last `ft->bundle_size() == 1` check?

src/hotspot/share/opto/inlinetypenode.cpp line 859:

> 857:   int vec_len = field_type->bundle_size();
> 858:   Node* value = gvn.zerocon(field_type->basic_type());
> 859:   int is_multifield = klass->declared_nonstatic_field_at(index)->is_multifield_base();

Rename `is_multifield` to `is_multifield_base` ? 
Or directly change to:

if (is_java_primitive(bt) &&
    klass->declared_nonstatic_field_at(index)->is_multifield_base() &&
    Matcher::match_rule_supported_vector(...))

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

PR Review Comment: https://git.openjdk.org/valhalla/pull/909#discussion_r1300823248
PR Review Comment: https://git.openjdk.org/valhalla/pull/909#discussion_r1300824685
PR Review Comment: https://git.openjdk.org/valhalla/pull/909#discussion_r1300825038
PR Review Comment: https://git.openjdk.org/valhalla/pull/909#discussion_r1300825532