Re: RFR: 8283689: Update the foreign linker VM implementation [v17]
On Fri, 13 May 2022 19:59:40 GMT, Jorn Vernee wrote:
>> 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());
>>
>> Still not indented properly.
>
> Shouldn't there be a 2-space indentation wrt the assert here? I could also
> indent all the arguments to be aligned with the format string, if that seems
> better.
It's preferred to indent multi-line argument lists on the column where argument
list starts.
-
PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8283689: Update the foreign linker VM implementation [v17]
On Fri, 13 May 2022 20:46:19 GMT, Vladimir Ivanov wrote: >> Shouldn't there be a 2-space indentation wrt the assert here? I could also >> indent all the arguments to be aligned with the format string, if that seems >> better. > > It's preferred to indent multi-line argument lists on the column where > argument list starts. assert(dst.is_single_reg(), "not a stack pair: (%s, %s), (%s, %s)", src.first()->name(), src.second()->name(), dst.first()->name(), dst.second()->name()); - PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8283689: Update the foreign linker VM implementation [v17]
On Fri, 13 May 2022 19:16:36 GMT, Vladimir Ivanov wrote:
>> Jorn Vernee has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 98 commits:
>>
>> - Merge branch 'master' into JEP-19-VM-IMPL2
>> - Undo spurious changes.
>> - Merge branch 'JEP-19-VM-IMPL2' of https://github.com/JornVernee/jdk into
>> JEP-19-VM-IMPL2
>> - Apply copyright year updates per request of @nick-arm
>>
>>Co-authored-by: Nick Gasson
>> - Fix overwritten copyright years.
>> - Missed 2 years
>> - Update Oracle copyright years
>> - Revert "Block async exceptions during upcalls"
>>
>>This reverts commit b29ad8f46732666f2d07e63ce8701b1eb7bed790.
>> - Block async exceptions during upcalls
>> - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
>> - ... and 88 more:
>> https://git.openjdk.java.net/jdk/compare/2c5d1362...f55b6c59
>
> src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5531:
>
>> 5529: }
>> 5530:
>> 5531: // On64 bit we will store integer like items to the stack as
>
> Missing space.
Oh, looks like i deleted the wrong space by accident.
> 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());
>
> Still not indented properly.
Shouldn't there be a 2-space indentation wrt the assert here? I could also
indent all the arguments to be aligned with the format string, if that seems
better.
-
PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8283689: Update the foreign linker VM implementation [v17]
On Thu, 12 May 2022 16:58:36 GMT, Jorn Vernee 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,
>> but it would be nice if we could get it into 19.
>>
>> 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 98 commits:
>
> - Merge branch 'master' into JEP-19-VM-IMPL2
> - Undo spurious changes.
> - Merge branch 'JEP-19-VM-IMPL2' of https://github.com/JornVernee/jdk into
> JEP-19-VM-IMPL2
> - Apply copyright year updates per request of @nick-arm
>
>Co-authored-by: Nick Gasson
> - Fix overwritten copyright years.
> - Missed 2 years
> - Update Oracle copyright years
> - Revert "Block async exceptions during upcalls"
>
>This reverts commit b29ad8f46732666f2d07e63ce8701b1eb7bed790.
> - Block async exceptions during upcalls
> - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2
> - ... and 88 more:
> https://git.openjdk.java.net/jdk/compare/2c5d1362...f55b6c59
Looks good.
src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 5531:
> 5529: }
> 5530:
> 5531: // On64 bit we will store integer like items to the stack as
Missing space.
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());
Still not indented properly.
-
Marked as reviewed by vlivanov (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7959
Re: RFR: 8283689: Update the foreign linker VM implementation [v17]
> 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, but it > would be nice if we could get it into 19. > > 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 98 commits: - Merge branch 'master' into JEP-19-VM-IMPL2 - Undo spurious changes. - Merge branch 'JEP-19-VM-IMPL2' of https://github.com/JornVernee/jdk into JEP-19-VM-IMPL2 - Apply copyright year updates per request of @nick-arm Co-authored-by: Nick Gasson - Fix overwritten copyright years. - Missed 2 years - Update Oracle copyright years - Revert "Block async exceptions during upcalls" This reverts commit b29ad8f46732666f2d07e63ce8701b1eb7bed790. - Block async exceptions during upcalls - Merge branch 'foreign-preview-m' into JEP-19-VM-IMPL2 - ... and 88 more: https://git.openjdk.java.net/jdk/compare/2c5d1362...f55b6c59 - Changes: https://git.openjdk.java.net/jdk/pull/7959/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7959&range=16 Stats: 6913 lines in 155 files changed: 2576 ins; 3219 del; 1118 mod Patch: https://git.openjdk.java.net/jdk/pull/7959.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7959/head:pull/7959 PR: https://git.openjdk.java.net/jdk/pull/7959
