RFR: 8184795: [MVT] non constant method handles can cause allocations in method handle combinators

[Roland Westrelin]

Hi Tobias,

Thanks for looking at this.

> src/share/vm/opto/castnode.cpp
> - It would be nice to move the allocation code in
> CheckCastPPNode::Ideal to valuetype.cpp or merge it with
> ValueTypeNode::allocate

Ok. Makes sense. Merging with ValueTypeNode::allocate doesn't seem
reasonable though because that one only runs during parsing.

>
> src/share/vm/opto/cfgnode.cpp
> - In line 1657, shouldn't this be:
>   assert(in(i) == NULL || (vtptr != NULL && phase->type(in(i))->higher_equal(vtptr)) || [...]

vtptr can be null if the first non null phi input is not a ValueTypePtr.

> - Why do you need the (in(1)->is_Phi()) check? Maybe add { } and a comment

The new allocation reuses the exception paths from the call above
it with extra regions and phis. I found that would break that logic.

> - Remove the linebreak in 1662
>
> src/share/vm/opto/type.cpp
> - Why did you change this from TypeKlassPtr to TypePtr?

Because we're returning either a class pointer or a pointer to a
value. So it's not always a TypeKlassPtr.

> I get some trailing whitespace errors (I have a save-action in my IDE to avoid this):
> - src/share/vm/opto/castnode.cpp:426: Trailing whitespace
> - test/compiler/valhalla/valuetypes/ValueTypeTestBench.java:2730: Trailing whitespace
>
> I also get some build failures on JPRT:
> /opt/jprt/T/P1/121328.tohartma/s/hotspot/src/share/vm/opto/valuetypenode.cpp:696:26: error: member access into incomplete type 'CheckCastPPNode'
>   ciValueKlass* vk = cast->type()->is_valuetypeptr()->value_type()->value_klass();
>                          ^
> /opt/jprt/T/P1/121328.tohartma/s/hotspot/src/share/vm/opto/valuetypenode.cpp:698:14: error: member access into incomplete type 'CheckCastPPNode'
>   assert(cast->in(1)->is_Proj(), "bad graph shape");

Ok. Will look at those.

>> This is a follow up to 8183137. test101 is an example of the problem
>> this change attempts to fix. test101 invokes a simple GWT combinator for
>> which one of the method handle targets is non constant. As a result, C2
>> doesn't know the type returned from that target and the change from
>> 8183137 that should sink ValueTypePtrNodes through phi to remove the
>> need for allocations doesn't trigger.
>> 
>> This is fixed by transforming:
>> 
>> (Phi ValueTypePtr#1 Node#2) to (Phi ValueTypePtr#1 CheckCastPP#2)
>
> Please add this as comment to cfgnode.cpp.

Ok.

>> Then pushing the CheckCastPP up through Phis until it reaches the non
>> constant call. The type of the return value is then known from the type
>> of the CheckCastPP. A ValueTypePtr can be created by adding projections
>> to the call for all values being returned. The oop edge is set by
>> allocating a new instance of the value type (that allocation is
>> hopefully useless and can be eliminated). The ValueTypePtr can then be
>> pushed down through Phis using the machinery from 8183137.
>> 
>> I had to disable compilation of LF as root if they return a method
>> handle value. The reason is that the logic in CheckCastPPNode::Ideal()
>> expects values to be returned in registers but is the called method is a
>> lambda form, I expect it could return a value as an instance. 
>
> Was this problem triggered by one of the tests? I'm seeing spurious
> failures on JPRT (without your changes) and wonder if that could be
> the cause.

It wasn't. I don't think it exists without that change.

>> The fix would require the logic in CheckCastPPNode::Ideal() to test if the
>> returned value is a class pointer or not (tagged or not).
>
> Okay, could you please file a follow up bug/enhancement for this?

Ok.

Roland.