Return value type fields in registers

Tue May 30 12:24:17 UTC 2017

Hi Roland,

I'm seeing build failures [1] on JPRT and the ValueTypeTestBench fails on Windows [2] and crashes with an assert [3] on my machine.

Here are some more detailed comments/questions regarding the changes:

interp_masm_x86.cpp
- remove new line in 1108 and line break in 1112

sharedRuntime_x86_64.cpp
- remove line break in 530

callGenerator.cpp
- line 132: Seprating -> Separating

callnode.cpp
- line 732: I think we should add a ValueTypeReturnedAsFields assert if > Parms+1

compile.cpp
- Why is the change in line 2869 necessary?
- 2692: "one of the value" -> "one of the values"

doCall.cpp
- line 662: please add an assert message like "expected value type but got ..."

ValueTypeTestBench
- It would be good to add a reference field to MyValue3 to extend coverage
- AlwaysIncrementalInlineOff and AlwaysIncrementalInlineOn can be removed

General:
I don't like the "tf()->range_sig() == tf()->range_cc()" or "range_cc == range_sig" checks. Couldn't we add a method to TypeFunc? For example, TypeFunc::returns_value_type_as_field().

Thanks,
Tobias

[1] Build failures on JPRT:

In file included from /scratch/opt/jprt/T/P1/101315.tohartma/s/hotspot/src/share/vm/gc/shared/collectedHeap.inline.hpp:36:0,
                 from /scratch/opt/jprt/T/P1/101315.tohartma/s/hotspot/src/share/vm/oops/oop.inline.hpp:31,
                 from /scratch/opt/jprt/T/P1/101315.tohartma/s/hotspot/src/share/vm/utilities/accessFlags.cpp:26:
/scratch/opt/jprt/T/P1/101315.tohartma/s/hotspot/src/share/vm/runtime/sharedRuntime.hpp:428:75: error: 'SigEntry' was not declared in this scope
                                                       const GrowableArray<SigEntry>& sig_extended,
                                                                           ^
/scratch/opt/jprt/T/P1/101315.tohartma/s/hotspot/src/share/vm/runtime/sharedRuntime.hpp:428:83: error: template argument 1 is invalid
                                                       const GrowableArray<SigEntry>& sig_extended,
                                                                                   ^
/scratch/opt/jprt/T/P1/101315.tohartma/s/hotspot/src/share/vm/runtime/sharedRuntime.hpp:435:51: error: 'SigEntry' was not declared in this scope
                               const GrowableArray<SigEntry>& sig_extended,
                                                   ^
/scratch/opt/jprt/T/P1/101315.tohartma/s/hotspot/src/share/vm/runtime/sharedRuntime.hpp:435:59: error: template argument 1 is invalid
                               const GrowableArray<SigEntry>& sig_extended,

[2] On Windows x64, the ValueTypeTestBench fails with:
Caused by: java.lang.RuntimeException: assertEquals: expected 1648712414171 to equal -3919
	at jdk.test.lib.Asserts.fail(Asserts.java:594)
	at jdk.test.lib.Asserts.assertEquals(Asserts.java:205)
	at jdk.test.lib.Asserts.assertEquals(Asserts.java:189)
	at jdk.test.lib.Asserts.assertEQ(Asserts.java:166)
	at compiler.valhalla.valuetypes.ValueTypeTestBench.test54_verifier(ValueTypeTestBench.java:1612)

[3] 
#  Internal Error (/oracle/valhalla/hotspot/src/share/vm/prims/methodHandles.cpp:532), pid=28572, tid=28573
#  Error: assert(is_basic_type_signature(bsig) || keep_last_arg) failed

Stack: [0x00007fb1c4dce000,0x00007fb1c4ecf000],  sp=0x00007fb1c4ecc770,  free space=1017k
Native frames: (J=compiled Java code, A=aot compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [libjvm.so+0x17410fc]  VMError::report_and_die(int, char const*, char const*, __va_list_tag*, Thread*, unsigned char*, void*, void*, char const*, int, unsigned long)+0x18c
V  [libjvm.so+0x1741ecf]  VMError::report_and_die(Thread*, char const*, int, char const*, char const*, __va_list_tag*)+0x2f
V  [libjvm.so+0xb2d23d]  report_vm_error(char const*, int, char const*, char const*, ...)+0xdd
V  [libjvm.so+0x130e4f2]  MethodHandles::lookup_basic_type_signature(Symbol*, bool, Thread*)+0x142
V  [libjvm.so+0x1159dcc]  LinkResolver::lookup_polymorphic_method(LinkInfo const&, Handle*, Handle*, Thread*)+0x50c
V  [libjvm.so+0x1161cc9]  LinkResolver::resolve_handle_call(CallInfo&, LinkInfo const&, Thread*)+0xb9
V  [libjvm.so+0x1162ed7]  LinkResolver::resolve_invoke(CallInfo&, Handle, constantPoolHandle const&, int, Bytecodes::Code, Thread*)+0x87
V  [libjvm.so+0xe9257e]  InterpreterRuntime::resolve_invokehandle(JavaThread*)+0x21e
V  [libjvm.so+0xe9e0f8]  InterpreterRuntime::resolve_from_cache(JavaThread*, Bytecodes::Code)+0xa8
j  compiler.valhalla.valuetypes.ValueTypeTestBench.test75()Qcompiler/valhalla/valuetypes/MyValue3;+4

On 29.05.2017 18:24, Roland Westrelin wrote:
> 
> Here is a new webrev for this. This one relies on Maurizio's patches to
> the method handles runtime (to generate lambda forms specialized to
> the value type supertype).
> 
> Interpreter methods now returns a pointer to the returned value type
> instance and the fields of the value types.
> 
> Compiled methods now returns a pointer to the returned value type's
> klass and the fields of the values types.
> 
> Interpreter->interpreter returns still go through a runtime call but
> that runtime call finds that the first argument is an oop and returns
> without doing anything else.
> 
> Compiled code->interpreter returns go through the same runtime call
> which uses the klass pointer that is returned to allocate a new value
> type instance that is then initialized from the values of the fields
> being returned.
> 
> Interpreter->compiled code and Compiled code->compiled code returns
> ignore the first returned value (the instance/klass pointer) and use the
> fields being returned directly unless the caller doesn't know the exact
> klass of the value type being returned (that is at a method handle
> linker call). In that case, the compiled code calls the same runtime
> call that the interpreter uses to allocate and initialize a new value
> type instance.
> 
> The webrev also fixes a couple 32 bit x86 build errors.
> 
> I noticed the ValueTypeTestBench test would sometimes get confused when
> parsing the output of the child JVM if an unexpected method is compiled
> (which can happen with method handle utility methods, at least with the
> new tests I added). I tried to make the parsing logic more robust. I
> also improved handling of AlwaysIncrementalInline by C2 so its behavior
> is closer to the non incremental inline case. As a consequence I removed
> some of the special case match rules for AlwaysIncrementalInline.
> 
> http://cr.openjdk.java.net/~roland/valhalla/returnconvention/webrev.01/
> 
> Roland.
> 