RFR(S): 8246377: [PPC64] Further improvements for assembler stop function

[Doerr, Martin]

Thanks. Pushed.

Best regards,
Martin


> -----Original Message-----
> From: Liu, Xin <xxinliu at amazon.com>
> Sent: Mittwoch, 3. Juni 2020 20:46
> To: Doerr, Martin <martin.doerr at sap.com>; hotspot-runtime-
> dev at openjdk.java.net; Schmidt, Lutz <lutz.schmidt at sap.com>; Andrew
> Haley (aph at redhat.com) <aph at redhat.com>
> Subject: Re: RFR(S): 8246377: [PPC64] Further improvements for assembler
> stop function
> 
> Hi, Martin,
> 
>     > You actually make TDI a variable-length instruction.  Why not just emit
>     > "(uintptr_t)msg" no matter what?
>     I don't make TDI a variable-length instruction. It's still 4 Byte like all
> instructions on PPC.
>     And MacroAssembler::stop is already variable-length. C2's HaltNode
> implementation too. I don't see any issue with that.
>     Why should we waste space and confuse disassembly if we have no
> information to add?
> ---
> Okay.
> 
>     > Particularly, "enum stop_msg_present = -0x8000" is correct but subtle.
> I'd
>     > like to learn from other reviewers.
>     Yeah, looks a bit ugly. I would have preferred 0x8000 in order to set the
> highest bit of the SI16 field, but it's signed so I need to use the negative value
> to avoid assertions complaining about integer overflow.
> ---
> Initially, I suspected that C++ can't guarantee that the enum is "signed".
> After pondering at the specification, it turns out that C++ standard actually
> can guarantee that. That's why I feel it's subtle.
> IMHO,  declaration with the explicit type is better, but it's only available in
> C++11.  "constexpr static int stop_msg_present = -0x8000".
> 
>     I think using this bit also makes the recognition of the instruction a bit more
> reliable before we read the const char* from the instruction stream.
> 
>     Btw. I have improved formatting in macroAssembler_ppc.hpp and in the os
> files as requested by Götz. Updated webrev in place.
> 
>     Are you ok with it? Can I add you as 2nd reviewer (no Reviewer status
> needed for that)?
>     Best regards,
>     Martin
> ---
> Yes, I am okay with it! thanks.
> 
> --lx
> 
> 
>     > -----Original Message-----
>     > From: Liu, Xin <xxinliu at amazon.com>
>     > Sent: Mittwoch, 3. Juni 2020 01:56
>     > To: Doerr, Martin <martin.doerr at sap.com>; hotspot-runtime-
>     > dev at openjdk.java.net; Schmidt, Lutz <lutz.schmidt at sap.com>; Andrew
>     > Haley (aph at redhat.com) <aph at redhat.com>
>     > Subject: Re: RFR(S): 8246377: [PPC64] Further improvements for
> assembler
>     > stop function
>     >
>     > Hi, Martin,
>     >
>     > I read it and it looks good to me, but it still needs a real reviewer approve.
>     >
>     > You actually make TDI a variable-length instruction.  Why not just emit
>     > "(uintptr_t)msg" no matter what?
>     > Particularly, "enum stop_msg_present = -0x8000" is correct but subtle.
> I'd
>     > like to learn from other reviewers.
>     >
>     > Thanks,
>     > --lx
>     >
>     >
>     > From: "Doerr, Martin" <martin.doerr at sap.com>
>     > Date: Tuesday, June 2, 2020 at 3:35 PM
>     > To: hotspot-runtime-dev <hotspot-runtime-dev at openjdk.java.net>,
>     > "Schmidt, Lutz" <lutz.schmidt at sap.com>, "Andrew Haley
>     > (aph at redhat.com)" <aph at redhat.com>, "Liu, Xin"
> <xxinliu at amazon.com>
>     > Subject: [EXTERNAL] RFR(S): 8246377: [PPC64] Further improvements for
>     > assembler stop function
>     >
>     > CAUTION: This email originated from outside of the organization. Do not
> click
>     > links or open attachments unless you can confirm the sender and know
> the
>     > content is safe.
>     >
>     > Hi,
>     >
>     > I’d like to have the improvements which we discussed for AArch64 also
> on
>     > PPC64 (+ a little more).
>     >
>     > Issue with details:
>     > https://bugs.openjdk.java.net/browse/JDK-8246377
>     >
>     > Webrev:
>     >
> http://cr.openjdk.java.net/~mdoerr/8246377_ppc64_improve_stop/webrev
>     > .00/
>     >
>     > Please review.
>     >
>     > Best regards,
>     > Martin
>     >
>