RFR(S) 8191437: AOT doesn't work easily after thread local handshakes

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR(S) 8191437: AOT doesn't work easily after thread local handshakes

dean.long
https://bugs.openjdk.java.net/browse/JDK-8191437

http://cr.openjdk.java.net/~dlong/8191437/webrev.hs/

http://cr.openjdk.java.net/~dlong/8191437/webrev.graal/


This change adds support for thread local handshakes to Graal for x64
and SPARC.  Graal generates a different instruction encoding on x64 than
C1/C2, so I had to account for that difference in
sharedRuntime_x86_64.cpp.  I'm including the Graal changes because I
would like to push them together with the HotSpot changes.


dl

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8191437: AOT doesn't work easily after thread local handshakes

Vladimir Kozlov
On 11/28/17 10:00 AM, [hidden email] wrote:
> https://bugs.openjdk.java.net/browse/JDK-8191437
>
> http://cr.openjdk.java.net/~dlong/8191437/webrev.hs/

VM changes looks fine to me.

>
> http://cr.openjdk.java.net/~dlong/8191437/webrev.graal/
>
>
> This change adds support for thread local handshakes to Graal for x64 and SPARC.  Graal generates a
> different instruction encoding on x64 than C1/C2, so I had to account for that difference in

I assume it works in all cases now (C1, C2, Graal generated code) ?

> sharedRuntime_x86_64.cpp.  I'm including the Graal changes because I would like to push them
> together with the HotSpot changes.

I would still suggest to do review and testing in Labs.

Thanks,
Vladimir

>
>
> dl
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8191437: AOT doesn't work easily after thread local handshakes

dean.long
On 11/28/17 4:12 PM, Vladimir Kozlov wrote:

> On 11/28/17 10:00 AM, [hidden email] wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8191437
>>
>> http://cr.openjdk.java.net/~dlong/8191437/webrev.hs/
>
> VM changes looks fine to me.
>

Thanks Vladimir.

>>
>> http://cr.openjdk.java.net/~dlong/8191437/webrev.graal/
>>
>>
>> This change adds support for thread local handshakes to Graal for x64
>> and SPARC.  Graal generates a different instruction encoding on x64
>> than C1/C2, so I had to account for that difference in
>
> I assume it works in all cases now (C1, C2, Graal generated code) ?

Yes.

>
>> sharedRuntime_x86_64.cpp.  I'm including the Graal changes because I
>> would like to push them together with the HotSpot changes.
>
> I would still suggest to do review and testing in Labs.
>

Yes, I waited until it was reviewed and pushed upstream to Graal before
sending out this RFR.

dl

> Thanks,
> Vladimir
>
>>
>>
>> dl
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8191437: AOT doesn't work easily after thread local handshakes

Vladimir Kozlov
On 11/28/17 4:49 PM, [hidden email] wrote:

> On 11/28/17 4:12 PM, Vladimir Kozlov wrote:
>
>> On 11/28/17 10:00 AM, [hidden email] wrote:
>>> https://bugs.openjdk.java.net/browse/JDK-8191437
>>>
>>> http://cr.openjdk.java.net/~dlong/8191437/webrev.hs/
>>
>> VM changes looks fine to me.
>>
>
> Thanks Vladimir.
>
>>>
>>> http://cr.openjdk.java.net/~dlong/8191437/webrev.graal/
>>>
>>>
>>> This change adds support for thread local handshakes to Graal for x64 and SPARC.  Graal generates a different
>>> instruction encoding on x64 than C1/C2, so I had to account for that difference in
>>
>> I assume it works in all cases now (C1, C2, Graal generated code) ?
>
> Yes.
>
>>
>>> sharedRuntime_x86_64.cpp.  I'm including the Graal changes because I would like to push them together with the
>>> HotSpot changes.
>>
>> I would still suggest to do review and testing in Labs.
>>
>
> Yes, I waited until it was reviewed and pushed upstream to Graal before sending out this RFR.

Good.

Thanks,
Vladimir

>
> dl
>
>> Thanks,
>> Vladimir
>>
>>>
>>>
>>> dl
>>>
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8191437: AOT doesn't work easily after thread local handshakes

Robbin Ehn
In reply to this post by dean.long
On 2017-11-28 19:00, [hidden email] wrote:
> https://bugs.openjdk.java.net/browse/JDK-8191437
>
> http://cr.openjdk.java.net/~dlong/8191437/webrev.hs/

Looks good, thanks for fixing!

/Robbin

>
> http://cr.openjdk.java.net/~dlong/8191437/webrev.graal/
>
>
> This change adds support for thread local handshakes to Graal for x64 and SPARC.  Graal generates a different instruction encoding on x64 than C1/C2, so I had to account for that difference in sharedRuntime_x86_64.cpp.  I'm including the Graal changes because I would like to push them together with the HotSpot changes.
>
>
> dl
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8191437: AOT doesn't work easily after thread local handshakes

Andrew Dinn
In reply to this post by dean.long
Hi Dean,

On 28/11/17 18:00, [hidden email] wrote:

> https://bugs.openjdk.java.net/browse/JDK-8191437
>
> http://cr.openjdk.java.net/~dlong/8191437/webrev.hs/
>
> http://cr.openjdk.java.net/~dlong/8191437/webrev.graal/
>
>
> This change adds support for thread local handshakes to Graal for x64
> and SPARC.  Graal generates a different instruction encoding on x64 than
> C1/C2, so I had to account for that difference in
> sharedRuntime_x86_64.cpp.  I'm including the Graal changes because I
> would like to push them together with the HotSpot changes.
Is there a reason for rejecting use of thread local handshakes in the
Graal AArch64 code rather than just adding support for it? The changes
to make AArch64 implement this are targetted for 10 and in review on
aarch64-port-dev

  https://bugs.openjdk.java.net/browse/JDK-8189596

http://mail.openjdk.java.net/pipermail/aarch64-port-dev/2017-November/005104.html

regards,


Andrew Dinn
-----------
Senior Principal Software Engineer
Red Hat UK Ltd
Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8191437: AOT doesn't work easily after thread local handshakes

Andrew Haley
On 29/11/17 11:15, Andrew Dinn wrote:
> Is there a reason for rejecting use of thread local handshakes in the
> Graal AArch64 code rather than just adding support for it? The changes
> to make AArch64 implement this are targetted for 10 and in review on
> aarch64-port-dev

They're not in review, they're in.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8191437: AOT doesn't work easily after thread local handshakes

dean.long
In reply to this post by Robbin Ehn
Thanks Robbin!

dl


On 11/29/17 12:23 AM, Robbin Ehn wrote:

> On 2017-11-28 19:00, [hidden email] wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8191437
>>
>> http://cr.openjdk.java.net/~dlong/8191437/webrev.hs/
>
> Looks good, thanks for fixing!
>
> /Robbin
>
>>
>> http://cr.openjdk.java.net/~dlong/8191437/webrev.graal/
>>
>>
>> This change adds support for thread local handshakes to Graal for x64
>> and SPARC.  Graal generates a different instruction encoding on x64
>> than C1/C2, so I had to account for that difference in
>> sharedRuntime_x86_64.cpp.  I'm including the Graal changes because I
>> would like to push them together with the HotSpot changes.
>>
>>
>> dl
>>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8191437: AOT doesn't work easily after thread local handshakes

dean.long
In reply to this post by Andrew Dinn
On 11/29/17 3:15 AM, Andrew Dinn wrote:

> Hi Dean,
>
> On 28/11/17 18:00, [hidden email] wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8191437
>>
>> http://cr.openjdk.java.net/~dlong/8191437/webrev.hs/
>>
>> http://cr.openjdk.java.net/~dlong/8191437/webrev.graal/
>>
>>
>> This change adds support for thread local handshakes to Graal for x64
>> and SPARC.  Graal generates a different instruction encoding on x64 than
>> C1/C2, so I had to account for that difference in
>> sharedRuntime_x86_64.cpp.  I'm including the Graal changes because I
>> would like to push them together with the HotSpot changes.
> Is there a reason for rejecting use of thread local handshakes in the
> Graal AArch64 code rather than just adding support for it? The changes
> to make AArch64 implement this are targetted for 10 and in review on
> aarch64-port-dev
>
>    https://bugs.openjdk.java.net/browse/JDK-8189596
>
> http://mail.openjdk.java.net/pipermail/aarch64-port-dev/2017-November/005104.html
>
> regards,

I didn't feel like I had the time or testing setup to do the aarch64
Graal support myself,  but yes I could have asked the aarch64 community
for help.  My bad.
The x64 and SPARC changes have already gone into Graal.  Does it make
sense to file a pull request at https://github.com/graalvm/graal for
aarch64 support?

dl

>
> Andrew Dinn
> -----------
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8191437: AOT doesn't work easily after thread local handshakes

dean.long
On 11/29/17 10:38 AM, [hidden email] wrote:

> On 11/29/17 3:15 AM, Andrew Dinn wrote:
>
>> Hi Dean,
>>
>> On 28/11/17 18:00, [hidden email] wrote:
>>> https://bugs.openjdk.java.net/browse/JDK-8191437
>>>
>>> http://cr.openjdk.java.net/~dlong/8191437/webrev.hs/
>>>
>>> http://cr.openjdk.java.net/~dlong/8191437/webrev.graal/
>>>
>>>
>>> This change adds support for thread local handshakes to Graal for x64
>>> and SPARC.  Graal generates a different instruction encoding on x64
>>> than
>>> C1/C2, so I had to account for that difference in
>>> sharedRuntime_x86_64.cpp.  I'm including the Graal changes because I
>>> would like to push them together with the HotSpot changes.
>> Is there a reason for rejecting use of thread local handshakes in the
>> Graal AArch64 code rather than just adding support for it? The changes
>> to make AArch64 implement this are targetted for 10 and in review on
>> aarch64-port-dev
>>
>>    https://bugs.openjdk.java.net/browse/JDK-8189596
>>
>> http://mail.openjdk.java.net/pipermail/aarch64-port-dev/2017-November/005104.html 
>>
>>
>> regards,
>
> I didn't feel like I had the time or testing setup to do the aarch64
> Graal support myself,  but yes I could have asked the aarch64
> community for help.  My bad.
> The x64 and SPARC changes have already gone into Graal.  Does it make
> sense to file a pull request at https://github.com/graalvm/graal for
> aarch64 support?
>

Aarch64 support is building, at least:

https://github.com/dean-long/graal/tree/aarch64-tls-poll

dl

> dl
>
>>
>> Andrew Dinn
>> -----------
>> Senior Principal Software Engineer
>> Red Hat UK Ltd
>> Registered in England and Wales under Company Registration No. 03798903
>> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8191437: AOT doesn't work easily after thread local handshakes

dean.long
On 11/29/17 12:14 PM, [hidden email] wrote:

> On 11/29/17 10:38 AM, [hidden email] wrote:
>
>> On 11/29/17 3:15 AM, Andrew Dinn wrote:
>>
>>> Hi Dean,
>>>
>>> On 28/11/17 18:00, [hidden email] wrote:
>>>> https://bugs.openjdk.java.net/browse/JDK-8191437
>>>>
>>>> http://cr.openjdk.java.net/~dlong/8191437/webrev.hs/
>>>>
>>>> http://cr.openjdk.java.net/~dlong/8191437/webrev.graal/
>>>>
>>>>
>>>> This change adds support for thread local handshakes to Graal for x64
>>>> and SPARC.  Graal generates a different instruction encoding on x64
>>>> than
>>>> C1/C2, so I had to account for that difference in
>>>> sharedRuntime_x86_64.cpp.  I'm including the Graal changes because I
>>>> would like to push them together with the HotSpot changes.
>>> Is there a reason for rejecting use of thread local handshakes in the
>>> Graal AArch64 code rather than just adding support for it? The changes
>>> to make AArch64 implement this are targetted for 10 and in review on
>>> aarch64-port-dev
>>>
>>>    https://bugs.openjdk.java.net/browse/JDK-8189596
>>>
>>> http://mail.openjdk.java.net/pipermail/aarch64-port-dev/2017-November/005104.html 
>>>
>>>
>>> regards,
>>
>> I didn't feel like I had the time or testing setup to do the aarch64
>> Graal support myself,  but yes I could have asked the aarch64
>> community for help.  My bad.
>> The x64 and SPARC changes have already gone into Graal.  Does it make
>> sense to file a pull request at https://github.com/graalvm/graal for
>> aarch64 support?
>>
>
> Aarch64 support is building, at least:
>
> https://github.com/dean-long/graal/tree/aarch64-tls-poll
>

Pull request:

https://github.com/graalvm/graal/pull/265

I could use some Aarch64 reviewers and testers.

dl

> dl
>
>> dl
>>
>>>
>>> Andrew Dinn
>>> -----------
>>> Senior Principal Software Engineer
>>> Red Hat UK Ltd
>>> Registered in England and Wales under Company Registration No. 03798903
>>> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8191437: AOT doesn't work easily after thread local handshakes

Andrew Haley
On 29/11/17 21:07, [hidden email] wrote:
> https://github.com/graalvm/graal/pull/265
>
> I could use some Aarch64 reviewers and testers.

I'm going to build this now.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8191437: AOT doesn't work easily after thread local handshakes

Andrew Haley
In reply to this post by dean.long
On 29/11/17 21:07, [hidden email] wrote:
> Pull request:
>
> https://github.com/graalvm/graal/pull/265
>
> I could use some Aarch64 reviewers and testers.

One thing breaks, and we need this hunk instead:

diff -r 4c25d37d8557 src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.aarch64/src/org/graalvm/compiler/hotspot/aarch64/AArch64HotSpotSafepointOp.java
--- a/src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.aarch64/src/org/graalvm/compiler/hotspot/aarch64/AArch64HotSpotSafepointOp.java Thu Nov 30 16:08:13 2017 +0100
+++ b/src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.aarch64/src/org/graalvm/compiler/hotspot/aarch64/AArch64HotSpotSafepointOp.java Thu Nov 30 16:48:18 2017 +0000
@@ -94,4 +105,14 @@
         }
     }

+    private static void emitThreadLocalPoll(CompilationResultBuilder crb, AArch64MacroAssembler masm, GraalHotSpotVMConfig config, boolean onReturn, Register thread, Register scratch, LIRFrameState state) {
+        assert config.threadPollingPageOffset >= 0;
+        masm.ldr(64, scratch, masm.makeAddress(thread, config.threadPollingPageOffset, 8));
+        crb.recordMark(onReturn ? config.MARKID_POLL_RETURN_FAR : config.MARKID_POLL_FAR);
+        if (state != null) {
+            crb.recordInfopoint(masm.position(), state, InfopointReason.SAFEPOINT);
+        }
+        masm.ldr(32, zr, AArch64Address.createBaseRegisterOnlyAddress(scratch));
+    }
+
 }

The only difference from what you pushed in MARKID_POLL_*FAR instead
of MARKID_POLL_*NEAR.

This is because we don't support MARKID_POLL_NEAR relocs in jvmci.
They can't happen, anyway: that's effectively dead code in AArch64's
Graal.

Thanks.

--
Andrew Haley
Java Platform Lead Engineer
Red Hat UK Ltd. <https://www.redhat.com>
EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Reply | Threaded
Open this post in threaded view
|

Re: RFR(S) 8191437: AOT doesn't work easily after thread local handshakes

dean.long
On 11/30/17 8:51 AM, Andrew Haley wrote:

> On 29/11/17 21:07, [hidden email] wrote:
>> Pull request:
>>
>> https://github.com/graalvm/graal/pull/265
>>
>> I could use some Aarch64 reviewers and testers.
> One thing breaks, and we need this hunk instead:
>
> diff -r 4c25d37d8557 src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.aarch64/src/org/graalvm/compiler/hotspot/aarch64/AArch64HotSpotSafepointOp.java
> --- a/src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.aarch64/src/org/graalvm/compiler/hotspot/aarch64/AArch64HotSpotSafepointOp.java Thu Nov 30 16:08:13 2017 +0100
> +++ b/src/jdk.internal.vm.compiler/share/classes/org.graalvm.compiler.hotspot.aarch64/src/org/graalvm/compiler/hotspot/aarch64/AArch64HotSpotSafepointOp.java Thu Nov 30 16:48:18 2017 +0000
> @@ -94,4 +105,14 @@
>           }
>       }
>
> +    private static void emitThreadLocalPoll(CompilationResultBuilder crb, AArch64MacroAssembler masm, GraalHotSpotVMConfig config, boolean onReturn, Register thread, Register scratch, LIRFrameState state) {
> +        assert config.threadPollingPageOffset >= 0;
> +        masm.ldr(64, scratch, masm.makeAddress(thread, config.threadPollingPageOffset, 8));
> +        crb.recordMark(onReturn ? config.MARKID_POLL_RETURN_FAR : config.MARKID_POLL_FAR);
> +        if (state != null) {
> +            crb.recordInfopoint(masm.position(), state, InfopointReason.SAFEPOINT);
> +        }
> +        masm.ldr(32, zr, AArch64Address.createBaseRegisterOnlyAddress(scratch));
> +    }
> +
>   }
>
> The only difference from what you pushed in MARKID_POLL_*FAR instead
> of MARKID_POLL_*NEAR.
>
> This is because we don't support MARKID_POLL_NEAR relocs in jvmci.
> They can't happen, anyway: that's effectively dead code in AArch64's
> Graal.

That makes sense.  Thanks for testing and fixing it.

dl

> Thanks.
>