On Mon, 9 May 2022 10:28:27 GMT, Jorn Vernee <jver...@openjdk.org> wrote:
>> Hi, >> >> This PR updates the VM implementation of the foreign linker, by bringing >> over commits from the panama-foreign repo. >> >> This is split off from the main JEP integration for 19, since we have >> limited resources to handle this. As such, this PR might fall over to 20. >> >> I've written up an overview of the Linker architecture here: >> http://cr.openjdk.java.net/~jvernee/docs/FL_Overview.html it might be useful >> to read that first. >> >> This patch moves from the "legacy" implementation, to what is currently >> implemented in the panama-foreign repo, except for replacing the use of >> method handle combinators with ASM. That will come in a later path. To >> recap. This PR contains the following changes: >> >> 1. VM stubs for downcalls are now generated up front, instead of lazily by >> C2 [1]. >> 2. the VM support for upcalls/downcalls now support all possible call >> shapes. And VM stubs and Java code implementing the buffered invocation >> strategy has been removed [2], [3], [4], [5]. >> 3. The existing C2 intrinsification support for the `linkToNative` method >> handle linker was no longer needed and has been removed [6] (support might >> be re-added in another form later). >> 4. Some other cleanups, such as: OptimizedEntryBlob (for upcalls) now >> implements RuntimeBlob directly. Binding to java classes has been rewritten >> to use javaClasses.h/cpp (this wasn't previously possible due to these java >> classes being in an incubator module) [7], [8], [9]. >> >> While the patch mostly consists of VM changes, there are also some Java >> changes to support (2). >> >> The original commit structure has been mostly retained, so it might be >> useful to look at a specific commit, or the corresponding patch in the >> [panama-foreign](https://github.com/openjdk/panama-foreign/pulls?q=is%3Apr) >> repo as well. I've also left some inline comments to explain some of the >> changes, which will hopefully make reviewing easier. >> >> Testing: Tier1-4 >> >> Thanks, >> Jorn >> >> [1]: >> https://github.com/openjdk/jdk/pull/7959/commits/048b88156814579dca1f70742061ad24942fd358 >> [2]: >> https://github.com/openjdk/jdk/pull/7959/commits/2fbbef472b4c2b4fee5ede2f18cd81ab61e88f49 >> [3]: >> https://github.com/openjdk/jdk/pull/7959/commits/8a957a4ed9cc8d1f708ea8777212eb51ab403dc3 >> [4]: >> https://github.com/openjdk/jdk/pull/7959/commits/35ba1d964f1de4a77345dc58debe0565db4b0ff3 >> [5]: >> https://github.com/openjdk/jdk/pull/7959/commits/4e72aae22920300c5ffa16fed805b62ed9092120 >> [6]: >> https://github.com/openjdk/jdk/pull/7959/commits/08e22e1b468c5c8f0cfd7135c72849944068aa7a >> [7]: >> https://github.com/openjdk/jdk/pull/7959/commits/451cd9edf54016c182dab21a8b26bd8b609fc062 >> [8]: >> https://github.com/openjdk/jdk/pull/7959/commits/4c851d2795afafec3a3ab17f4142ee098692068f >> [9]: >> https://github.com/openjdk/jdk/pull/7959/commits/d025377799424f31512dca2ffe95491cd5ae22f9 > > Jorn Vernee has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 21 commits: > > - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2 > - Remove unneeded ComputeMoveOrder > - Remove comment about native calls in lcm.cpp > - 8284072: foreign/StdLibTest.java randomly crashes on MacOS/AArch64 > > Reviewed-by: jvernee, mcimadamore > - Update riscv and arm stubs > - Remove spurious ProblemList change > - Pass pointer to LogStream > - Polish > - Replace TraceNativeInvokers flag with unified logging > - Fix other platforms, take 2 > - ... and 11 more: > https://git.openjdk.java.net/jdk/compare/3c88a2ef...43fd1b91 Nice work! Looks good. Some minor comments/questions follow. src/hotspot/cpu/aarch64/frame_aarch64.cpp line 379: > 377: // need unextended_sp here, since normal sp is wrong for interpreter > callees > 378: return reinterpret_cast<OptimizedEntryBlob::FrameData*>( > 379: reinterpret_cast<char*>(frame.unextended_sp()) + > in_bytes(_frame_data_offset)); Maybe use `address` instead of `char*`? src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5531: > 5529: } > 5530: > 5531: // On 64 bit we will store integer like items to the stack as Time for a cleanup? `64 bit` vs `64bit`, `abi`, `Aarch64`. src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5547: > 5545: } else if (dst.first()->is_stack()) { > 5546: // reg to stack > 5547: // Do we really have to sign extend??? Obsolete? Remove? src/hotspot/cpu/aarch64/universalUpcallHandler_aarch64.cpp line 306: > 304: intptr_t exception_handler_offset = __ pc() - start; > 305: > 306: // Native caller has no idea how to handle exceptions, Can you elaborate, please, how it is expected to work in presence of asynchronous exceptions? I'd expect to see a code which unconditionally clears pending exception with an assertion that verifies that the exception is of expected type. src/hotspot/cpu/x86/foreign_globals_x86.hpp line 30: > 28: #include "utilities/growableArray.hpp" > 29: > 30: class outputStream; Redundant declaration? src/hotspot/cpu/x86/foreign_globals_x86_64.cpp line 52: > 50: > 51: objArrayOop inputStorage = > jdk_internal_foreign_abi_ABIDescriptor::inputStorage(abi_oop); > 52: loadArray(inputStorage, INTEGER_TYPE, abi._integer_argument_registers, > as_Register); `loadArray` helper looks a bit misleading. In presence of `javaClass`-style accessors, it misleadingly hints that it refers to some Java-level operation/entity, though what it does it parses register list representation backed by a Java array. I suggest to rename it to something like `parse_argument_registers_array()`). src/hotspot/cpu/x86/macroAssembler_x86.cpp line 933: > 931: } else { > 932: assert(dst.is_single_reg(), "not a stack pair: (%s, %s), (%s, %s)", > 933: src.first()->name(), src.second()->name(), dst.first()->name(), > dst.second()->name()); Wrong indentation. src/hotspot/cpu/x86/sharedRuntime_x86_64.cpp line 36: > 34: #include "code/nativeInst.hpp" > 35: #include "code/vtableStubs.hpp" > 36: #include "compiler/disassembler.hpp" Redundant includes? No new code added in the file. src/hotspot/share/c1/c1_GraphBuilder.cpp line 4230: > 4228: > 4229: case vmIntrinsics::_linkToNative: > 4230: print_inlining(callee, "Native call", /*success*/ false); Since the message is appended, lower case is preferred:`"native call"`. src/hotspot/share/code/codeBlob.hpp line 754: > 752: class ProgrammableUpcallHandler; > 753: > 754: class OptimizedEntryBlob: public RuntimeBlob { What's the motivation to move `OptimizedEntryBlob` up in the hierarchy (from `BufferBlob` to `RuntimeBlob`)? src/hotspot/share/opto/callGenerator.cpp line 1131: > 1129: > 1130: case vmIntrinsics::_linkToNative: > 1131: print_inlining_failure(C, callee, jvms->depth() - 1, jvms->bci(), Why is it unconditionally reported as inlining failure? src/hotspot/share/prims/foreign_globals.cpp line 147: > 145: // based on ComputeMoveOrder from x86_64 shared runtime code. > 146: // with some changes. > 147: class ForeignCMO: public StackObj { Considering how seldom it is used, I don't see much value in abbreviating it. Also, the comment is misleading: there's no such entity as `ComputeMoveOrder` in the code. And `compute_move_order` is completely removed by this change. src/hotspot/share/prims/foreign_globals.cpp line 217: > 215: > 216: public: > 217: ForeignCMO(int total_in_args, const VMRegPair* in_regs, int > total_out_args, VMRegPair* out_regs, I propose to turn it into a trivial ctor and move all the logic into a helper static function which returns the computed moves. src/hotspot/share/prims/foreign_globals.hpp line 35: > 33: #include CPU_HEADER(foreign_globals) > 34: > 35: class CallConvClosure { Just a question on terminology: why is it called a `Closure`? src/hotspot/share/prims/foreign_globals.hpp line 62: > 60: > 61: > 62: class JavaCallConv : public CallConvClosure { Does it really worth to abbreviate `CallingConvention` to `CallConv`? src/java.base/share/classes/jdk/internal/foreign/abi/SharedUtils.java line 313: > 311: MethodType newType = oldType.dropParameterTypes(destIndex, > destIndex + 1); > 312: int[] reorder = new int[oldType.parameterCount()]; > 313: if (destIndex < sourceIndex) Misses braces. src/java.base/share/classes/jdk/internal/foreign/abi/aarch64/AArch64Architecture.java line 169: > 167: stackAlignment, > 168: shadowSpace, > 169: targetAddrStorage, retBufAddrStorage); Wrong indentation. src/java.base/share/classes/jdk/internal/foreign/abi/x64/X86_64Architecture.java line 156: > 154: stackAlignment, > 155: shadowSpace, > 156: targetAddrStorage, retBufAddrStorage); Wrong indentation. ------------- PR: https://git.openjdk.java.net/jdk/pull/7959
