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

[Liu, Xin]

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
    >