On Wed, 9 Dec 2020 08:20:38 GMT, Nick Gasson <[email protected]> wrote:
> This is more-or-less a straight port of the x86 code to AArch64. > GraphKit::make_native_call() calls SharedRuntime::make_native_invoker() > to generate a blob that jumps to the native function entry point. This > simply switches the thread state from Java to native and handles the > safepoint poll on return from native code. > > AArch64 suffers the same problem as x86 in JDK-8251047 where R29 (frame > pointer) may hold a live oop over the MachCallNative node. Normally this > would be saved by RegisterSaver::save_live_registers() but the native > invoker blob is not a "proper" stub routine so doesn't have an oop map. > I copied the x86 solution to this where the frame pointer register is > saved to the stack and a pointer to that is stored in the frame anchor. > This is then read back and added to the register map when walking the > stack. I saw in the PR comments [1] that this might be a temporary fix, > but I'm not sure if there's any plan to change that now? > > Another potential fix might be to change the C2 register definition of > R29 and R29_H to be save-on-call as below. This works for the > TestStackWalk.java test case but I don't know whether it has other > unintended side-effects. > > reg_def R29 ( SOC, NS, Op_RegI, 29, r29->as_VMReg() ); // fp > reg_def R29_H ( SOC, NS, Op_RegI, 29, r29->as_VMReg()->next()); > > JMH results from jdk/incubator/foreign/CallOverhead.java to show it's > working: > > -Djdk.internal.foreign.ProgrammableInvoker.USE_INTRINSICS=false: > > Benchmark Mode Cnt Score Error > Units > CallOverhead.jni_blank avgt 30 51.450 ? 0.363 > ns/op > CallOverhead.jni_identity avgt 30 54.145 ? 0.627 > ns/op > CallOverhead.panama_args10 avgt 30 1914.431 ? 73.771 > ns/op > CallOverhead.panama_args5 avgt 30 1394.274 ? 49.369 > ns/op > CallOverhead.panama_blank avgt 30 872.878 ? 20.716 > ns/op > CallOverhead.panama_blank_trivial avgt 30 873.852 ? 21.350 > ns/op > CallOverhead.panama_identity avgt 30 1058.729 ? 20.229 > ns/op > CallOverhead.panama_identity_memory_address avgt 30 1041.648 ? 22.930 > ns/op > CallOverhead.panama_identity_struct avgt 30 3212.483 ? 151.627 > ns/op > CallOverhead.panama_identity_trivial avgt 30 1056.398 ? 18.779 > ns/op > > -Djdk.internal.foreign.ProgrammableInvoker.USE_INTRINSICS=true: > > Benchmark Mode Cnt Score Error > Units > CallOverhead.jni_blank avgt 30 51.519 ? 0.345 > ns/op > CallOverhead.jni_identity avgt 30 54.689 ? 0.687 > ns/op > CallOverhead.panama_args10 avgt 30 42.856 ? 0.760 > ns/op > CallOverhead.panama_args5 avgt 30 42.192 ? 0.712 > ns/op > CallOverhead.panama_blank avgt 30 41.934 ? 0.349 > ns/op > CallOverhead.panama_blank_trivial avgt 30 2.806 ? 0.545 > ns/op > CallOverhead.panama_identity avgt 30 44.043 ? 0.398 > ns/op > CallOverhead.panama_identity_memory_address avgt 30 45.021 ? 0.409 > ns/op > CallOverhead.panama_identity_struct avgt 30 3206.829 ? 175.750 > ns/op > CallOverhead.panama_identity_trivial avgt 30 7.849 ? 1.651 > ns/op > > [1] https://github.com/openjdk/panama-foreign/pull/279#issuecomment-679172326 src/hotspot/cpu/aarch64/aarch64.ad line 16057: > 16055: > 16056: format %{ "CALL, native $meth" %} > 16057: It might be better to expand this a little to indicate a near or a far call, if that's possible. src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 3156: > 3154: > 3155: // Store a pointer to the previous R29 saved on the stack as it may > 3156: // contain an oop if PreserveFramePointer is off. This value is This is a bit confusing: please say "R29 (RFP)" here. src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 3186: > 3184: // Make sure that native code does not change SVE vector length. > 3185: __ verify_sve_vector_length(); > 3186: } Do we have to surround every call to verify_sve_vector_length() with if (UseSVE > 0) ? Why is that check not inside verify_sve_vector_length() ? What is the meaning of the > 0 comparison? Can it be negative? So many questions. :-) src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 3227: > 3225: __ mov(c_rarg0, rthread); > 3226: #ifndef PRODUCT > 3227: assert(frame::arg_reg_save_area_bytes == 0, "not expecting frame reg > save area"); I don't think we need #ifndef PRODUCT here. src/hotspot/cpu/aarch64/sharedRuntime_aarch64.cpp line 3098: > 3096: MacroAssembler* masm = _masm; > 3097: if (reg->is_Register()) { > 3098: __ push(RegSet::of(reg->as_Register()), sp); Is this right? SP is 16-aligned, so this will use 16 bytes of stack for every 8-byte register. Does it get used a lot? ------------- PR: https://git.openjdk.java.net/jdk/pull/1711
