Re: RFR: 8283689: Update the foreign linker VM implementation [v17]

2022-05-13 Thread Vladimir Ivanov
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]

2022-05-13 Thread Vladimir Ivanov
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]

2022-05-13 Thread Jorn Vernee
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]

2022-05-13 Thread Vladimir Ivanov
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]

2022-05-12 Thread Jorn Vernee
> 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=7959=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