RFR(L): 8193257: PPC64, s390 implementation for Thread-local handshakes

[Lindenmaier, Goetz]

Hi, 

I would prefer
"Check whether safepoint is requested and if so branch to code suspending the thread."
The word 'poll' isn't very precise to describe what is happening here.

Other stuff all fine. Reviewed. No new webrev needed.

Best regards,
  Goetz. 

> -----Original Message-----
> From: Doerr, Martin
> Sent: Dienstag, 12. Dezember 2017 11:08
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-dev
> developers (hotspot-dev at openjdk.java.net) <hotspot-
> dev at openjdk.java.net>; Schmidt, Lutz <lutz.schmidt at sap.com>
> Subject: RE: RFR(L): 8193257: PPC64, s390 implementation for Thread-local
> handshakes
> 
> Hi Götz,
> 
> thanks reviewing. Please see my answers inline.
> 
> Best regards,
> Martin
> 
> 
> > I see you enabled it per default?
> Yes. I have tried to keep the implementation as close to the other platforms
> as possible. x86, SPARC and aarch64 have it enabled by default, too.
> 
> > macroAssembler_ppc/s390.cpp:
> > MacroAssembler::safepoint_poll()
> > Could you add some comment that says what this is doing?
> > As it's not doing a safepoint_poll ... its just preparing it, right?
> > Maybe also "slow_target" should be "do_safepoint".
> I agree with that the label name "do_safepoint" would be more
> comprehensible, but I'd prefer to use the same name as x86, SPARC and
> aarch64.
> What the function safepoint_poll does is it simply performs the poll by
> checking the poll bit + conditional branch to the safepoint code. Would you
> like to see a comment like "Perform poll and branch if safepoint requested"?
> 
> > templateInterpreterGenerator_ppc.cpp:
> > Why do you change memory ordering from release() to lwsync()?
> This is not a functional change. release() emits an lwsync() instruction. I have
> changed it to emphasize that we're using it for acquire and release in one
> instruction. release() hides that we use it for acquire, too.
> 
> > sharedRuntime_ppc/s390.cpp
> > If I understand right you skip the safepoint instruction.
> > Why that? Could you please document that this differs
> > from the other platforms here?
> The s390 code does exactly the same thing as on the other platforms. It
> adjusts to PC to point to the instruction following the poll instruction. This is
> part of the new concept which needs to be implemented on all platforms
> supporting local handshakes. The difference to PPC for example is that s390
> has different instruction sizes (2, 4 and 6 bytes) and I implemented it in a way
> that it works with any poll instruction regardless of its size.
> Would you like to see a comment like "Support poll instruction with any
> size"?