RFR(L): 8185556: [MVT] C2 compiler support for non-flattened value type fields

[Tobias Hartmann]

Hi Roland,

thanks for looking at this!

On 26.09.2017 11:20, Roland Westrelin wrote:
> sharedRuntime_x86_64.cpp:
>   486     case T_VALUETYPE:
> 
> Why is that needed? Shouldn't we only see T_VALUETYPEPTR here?

Right, that's not necessary. Removed.

> I'm also confused by that comment:
> 
> 556       // TODO change this when we use T_VALUETYPEPTR
> 
> in signature.cpp

When generating the extended signature in AdapterHandlerLibrary::get_adapter0, we used T_VALUETYPE for value type 
arguments when ValueTypePassFieldsAsArgs was disabled. However, we should really use T_VALUETYPEPTR because we are 
passing a reference. I added this workaround and the TODO to later clean that up.

I now fixed the code in get_adapter0, removed the workaround and added an assert to SigEntry::fill_sig_bt().

> callGenerator.cpp:
> 
> 442         Node* ctl = map->control();
> 443         arg = ValueTypeNode::make(gvn, ctl, map->memory(), arg);
> 444         map->set_control(ctl);
> 
> Why the set_control()?

That's necessary because ValueTypeNode::make may update ctl when adding a null check for a non-flattened value type 
field load. We need to use the new control when continuing with parsing.

> That comment confuses me, too:
> 
> 2117   // TODO enable when using T_VALUETYPEPTR
> 
> in escape.cpp

The field_type() of the FieldDescriptor for a non-flattened value type field is T_VALUETYPE (not T_VALUETYPEPTR) 
although we store a reference. When looking up the type of a ciField, we may now find T_VALUETYPE which needs to be 
treated like a reference.

To fix this properly, we need to change ciField construction in ciInstanceKlass::compute_nonstatic_fields_impl() to use 
the type T_VALUETYPEPTR for non-flattened value type fields. I'll fix this with my next change.

There are two other places where this matters and where I added a corresponding TODO:
- ciValueKlass.cpp, line 90
- deoptimization.cpp, line 1039

> In graphKit.cpp:
> 
> 3395 Node* GraphKit::new_instance(Node* klass_node,
> 3396                              Node* extra_slow_test,
> 3397                              Node* *return_size_val,
> 3398                              bool deoptimize_on_exception,
> 3399                              ValueTypeBaseNode* value_node) {
> 
> when can we pass a ValueTypePtrNode here?

Yes, when calling ValueTypeBaseNode::allocate() with a ValueTypePtrNode from CheckCastPPNode::Ideal(). It shouldn't be a 
problem because we just attach the ValueTypePtrNode to the AllocatedNode which goes away after macro node expansion.

If necessary, we could extend ValueTypeNode::remove_redundant_allocations() to also work with ValueTypePtrNodes.

> Using GraphKit with an igvn should only be safe if _delay_transform is
> true, right? Should there be an assert for _delay_transform in the
> GraphKit constructor?

Correct, I've added an assert to the GraphKit constructor.

> Also, usually after the graph is constructed with GraphKit, there is a
> pass of PhaseRemoveUseless to disconnect useless nodes. Your new use of
> GraphKit during igvn is not followed by PhaseRemoveUseless. I wonder if
> it's a problem and if some untransformed node could stay connected to
> graph.

Why is that different to the version that does not use the GraphKit?

All nodes that were added by the GraphKit should end up being processed by a subsequent PhaseIterGVN::optimize() because 
they are guaranteed to be added to the worklist - which is verified by PhaseIterGVN::verify_PhaseIterGVN(). IGVN should 
be "smart" enough to kill dead nodes, right?

> In phaseX.cpp, PhaseIterGVN::transform(), why the change?

I've mentioned this in the RFR:
"If we use the GraphKit with IGVN and _delay_transform, we may transform the same node multiple times. Don't use 
register_new_node_with_optimizer because it tries to set the type multiple times."

So with my change, PhaseIterGVN::transform() does exactly the same as before but does not fail if called multiple times 
on the same node if _delay_transform == true.

> I find the overloaded ValueTypeNode::make() calls quite confusing after
> all. I think renaming them to something that's more descriptive of what
> they do would be better.

Yes, I agree but would prefer to do that with a separate change. I'll take care of it with my next change.

Updated and consolidated webrev:
http://cr.openjdk.java.net/~thartmann/8185556/webrev.01/

Thanks,
Tobias