RFR(XS): Provide information when hitting a HaltNode for architectures other than x86

[Christian Hagedorn]

Hi Ningsheng

On 06.05.20 08:35, Ningsheng Jian wrote:
> Currently in AArch64 MacroAssembler::stop(), it will generate many 
> register saving instructions by pusha() before calling to debug64(). But 
> I think debug64() only uses the regs[] and pc arguments when 
> ShowMessageBoxOnError is on. Maybe we should at least only do the saving 
> and pc arg passing when ShowMessageBoxOnError is on in 
> MacroAssembler::stop(), as what x86 does in macroAssembler_x86.cpp?

That's a good point. The change from Assembler::ud2() to 
MacroAssembler::stop() for a HaltNode in JDK-8225653 indeed resulted in 
some performance regressions for x86 since MacroAssembler::debug64() set 
up those arguments [1]. Especially, it called Assembler::pusha() for 
64-bit which emits a move instructions for each of the 16 registers. 
Even though the code was never called, it had a bad influence on code 
cache space which had a bad influence on performance. The fix was to 
only do it if ShowMessageBoxOnError was set. So, I think it's indeed a 
good idea to also do the same for other architectures.

Best regards,
Christian

[1] https://bugs.openjdk.java.net/browse/JDK-8231720

> 
> 
> Thanks,
> Ningsheng
> 
> On 5/5/20 7:26 AM, Liu, Xin wrote:
>> Hi, Martin,
>>
>> Thank you to review it and build it on s390 and PPC!
>>
>> If I delete size(4) in ppc.ad,  hotspot can work out the correct size 
>> of instruction chunk, can't it?
>> I found most of instructions in ppc.ad have size(xx), but there're a 
>> couple of exceptions:  cacheWB & cacheWBPreSync.
>>
>> I think it should be in product hotspot.  Here are my arguments.
>> 1.  Crash reports of release build are more common.
>> Some customers even don't bother trying again with a debug build.
>>
>> Let me take the crash report on aarch64 as an example. I paste the 
>> comparison before and after.
>> https://bugs.openjdk.java.net/browse/JDK-8230552?focusedCommentId=14334977&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14334977 
>>
>>
>> Without stop(_halt_reason), what we know is "there's a bug in 
>> C2-generated code". If the method is big,  which is very likely 
>> because of inlining, it's easy to get lost.
>>
>> I feel it's more helpful with the patch. We can locate which HaltNode 
>> it is by searching _halt_reason in codebase.
>> Hopefully, we can find the culprit method from "Compilation events".
>>
>> 2. HaltNode is rarely generated and will be removed if it's dead.
>> IMHO, the semantic of that Node is "halt". If It remains after 
>> optimizer or lowering to mach-nodes,  something wrong and 
>> unrecoverable happened in the compilers. After we fix the compiler 
>> bug,  it should be gone.  That's is too say, it shouldn't cause any 
>> problem about code size in ideal cases.
>>
>> In reality, I observe that a HaltNode always follows the uncommon_trap 
>> call.  Christian also observed that in JDK-8022574.
>> Isn't uncommon trap a one-way ticket for all architectures? I feel the 
>> control flow never returns after uncommon_trap, why do we generate a 
>> HaltNode after that? Nevertheless, it's a separated issue.
>>
>> Let me make another revision to fix PPC and I found that sparc.ad is 
>> gonna gone.
>>
>> Thanks,
>> --lx
>>
>>
>> On 5/4/20, 1:47 PM, "Doerr, Martin" <martin.doerr at sap.com> wrote:
>>
>>      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 lx,
>>
>>      the size attribute is wrong on PPC64 (stop is larger than 4 
>> Bytes). S390 builds fine.
>>      I've only run the build. No tests.
>>
>>      Should this feature be debug-only?
>>      Do we want the lengthy code emitted in product build?
>>
>>      Best regards,
>>      Martin
>>
>>
>>      > -----Original Message-----
>>      > From: hotspot-compiler-dev <hotspot-compiler-dev-
>>      > bounces at openjdk.java.net> On Behalf Of Liu, Xin
>>      > Sent: Donnerstag, 30. April 2020 06:03
>>      > To: hotspot-compiler-dev at openjdk.java.net
>>      > Subject: RFR(XS): Provide information when hitting a HaltNode for
>>      > architectures other than x86
>>      >
>>      > Hi,
>>      >
>>      > Could you review this small patch?  It unifies codegen of 
>> HaltNode for other
>>      > architectures.
>>      > JBS: https://bugs.openjdk.java.net/browse/JDK-8230552
>>      > Webrev: https://cr.openjdk.java.net/~xliu/8230552/00/webrev/
>>      >
>>      > I tested on aarch64.  It generates the same crash report as 
>> x86_64 when it
>>      > does hit HaltNode.  Halt reason is displayed. I paste report on 
>> the JBS.
>>      > I ran hotspot:tier1 on aarch64 fastdebug build.  It passed 
>> except for 3
>>      > relevant failures[1].
>>      >
>>      > I plan to do that on aarch64 only, but it’s trivial on other 
>> architectures, so I
>>      > bravely modified them all.  May I invite s390, SPARC arm32 
>> maintainers take a
>>      > look at it?
>>      > If it goes through the review, I hope a sponsor can help me to 
>> push the
>>      > submit repo and see if it works.
>>      >
>>      > [1] those 3 tests failed on aarch64 with/without my changes.
>>      > gc/shenandoah/mxbeans/TestChurnNotifications.java#id2
>>      > gc/shenandoah/mxbeans/TestChurnNotifications.java#id1
>>      > gc/shenandoah/mxbeans/TestPauseNotifications.java#id1
>>      >
>>      > thanks,
>>      > -lx
>>      >
>>
>>
>