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

[Tobias Hartmann]

Hi Roland,

On 18.07.2017 15:00, Roland Westrelin wrote:
> http://cr.openjdk.java.net/~roland/8184795/webrev.00/

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

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)) || [...]
- Why do you need the (in(1)->is_Phi()) check? Maybe add { } and a comment
- Remove the linebreak in 1662

src/share/vm/opto/type.cpp
- Why did you change this from TypeKlassPtr to TypePtr?

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");
             ^
> 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.

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

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

Thanks,
Tobias