RFR: 8208171: PPC64: Enrich SLP support
Doerr, Martin
martin.doerr at sap.com
Wed Sep 5 17:45:22 UTC 2018
Hi Gustavo,
thank you for your detailed explanation. I wonder what happens with the registers when VSX gets disabled, but the regs are read again many context switches later. But I guess this is solved somehow.
I'm getting different incorrect results every time I run the test on some machines, while other machines always compute the correct result and the test passes.
But I found out that the problem shows up with different kernel versions (4.4.0-101-generic, 3.10.0-693.1.1.el7.ppc64le, 4.4.126-94.22-default). So I guess it's rather unlikely that the problem is only caused by the OS.
After more investigation, it rather looks like v0 is not preserved across the safepoint:
vs32 = v0, vs36 = v4, vs40 = v8
0x00007fff6813e6d0: extsw r15,r17
0x00007fff6813e6d4: rldic r18,r17,2,30
0x00007fff6813e6d8: add r18,r21,r18
0x00007fff6813e6dc: addi r20,r18,16
0x00007fff6813e6e0: addi r18,r18,16
0x00007fff6813e6e4: lxvd2x vs36,0,r18
0x00007fff6813e6e8: vaddfp v4,v4,v0
0x00007fff6813e6ec: rldicr r15,r15,2,61
0x00007fff6813e6f0: add r15,r21,r15
0x00007fff6813e6f4: addi r18,r15,32
0x00007fff6813e6f8: addi r15,r15,32
0x00007fff6813e6fc: lxvd2x vs40,0,r15 ;*faload {reexecute=0 rethrow=0 return_oop=0}
; - compiler.runtime.safepoints.TestRegisterRestoring::increment at 21 (line 62)
0x00007fff6813e700: stxvd2x vs36,0,r20
0x00007fff6813e704: vaddfp v4,v8,v0
0x00007fff6813e708: stxvd2x vs36,0,r18 ;*fastore {reexecute=0 rethrow=0 return_oop=0}
; - compiler.runtime.safepoints.TestRegisterRestoring::increment at 24 (line 62)
0x00007fff6813e70c: addi r17,r17,8 ;*iinc {reexecute=0 rethrow=0 return_oop=0}
; - compiler.runtime.safepoints.TestRegisterRestoring::increment at 25 (line 61)
0x00007fff6813e710: cmpw cr5,r17,r24
0x00007fff6813e714: blt cr5,0x00007fff6813e6d0 ;*goto {reexecute=0 rethrow=0 return_oop=0}
; - compiler.runtime.safepoints.TestRegisterRestoring::increment at 28 (line 61)
;; B15: # B14 B16 <- B14 Freq: 12356.3
0x00007fff6813e718: ld r15,288(r16) ; ImmutableOopMap{R21=Oop }
;*goto {reexecute=1 rethrow=0 return_oop=0}
; - compiler.runtime.safepoints.TestRegisterRestoring::increment at 28 (line 61)
0x00007fff6813e71c: tdlgei r15,8 ;*goto {reexecute=0 rethrow=0 return_oop=0}
; - compiler.runtime.safepoints.TestRegisterRestoring::increment at 28 (line 61)
; {poll}
0x00007fff6813e720: cmpw cr6,r17,r24
0x00007fff6813e724: blt cr6,0x00007fff6813e6d0
At the end of the method, I see v4_float = {10000, 10000, 10000, 10000} on machines on which the test passes.
On a machine on which it fails, e.g. v4_float = {0xffffffff, 0x8a296200, 0xffffffff, 0xffffffff}
I thought we had already checked saving and restoring vector registers at safepoints, but seems like we have missed something.
Best regards,
Martin
-----Original Message-----
From: Gustavo Romero <gromero at linux.vnet.ibm.com>
Sent: Mittwoch, 5. September 2018 18:21
To: Doerr, Martin <martin.doerr at sap.com>; Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Michihiro Horie <HORIE at jp.ibm.com>
Cc: hotspot compiler <hotspot-compiler-dev at openjdk.java.net>; hotspot-dev at openjdk.java.net
Subject: Re: RFR: 8208171: PPC64: Enrich SLP support
Hi Martin,
On 09/03/2018 02:18 PM, Doerr, Martin wrote:
> Hi Gustavo and Michihiro,
>
> we noticed jtreg test failures when using this change:
> compiler/runtime/safepoints/TestRegisterRestoring.java
> compiler/runtime/Test7196199.java
>
> TestRegisterRestoring is a simple test which returns arbitrary results instead of 10000.
Just to confirm I understood the description correctly:
Where you able to check it's returning random values for the
array instead of 10_000 or you just checked that test failed?
Also, did you pass -XX:-SuperwordUseVSX when it failed? I'm
asking because I'm able to fail that test due to a timeout, but not sure
if it's the same you got there. Look (I'm using the same kernel as yours):
http://cr.openjdk.java.net/~gromero/logs/slp_failure0.txt
Thank you.
Best regards,
Gustavo
> We didn't see it on all machines, so it might be an issue with saving&restoring VR registers in the signal handler.
> The machine which I have used has "SUSE Linux Enterprise Server 12 SP3" with kernel 4.4.126-94.22-default.
>
> That's what I found out so far. Maybe you have an idea?
>
> I also noticed that "-XX:-SuperwordUseVSX" crashes with bad ad file when your patch is applied. Looks like matching the vector nodes needs to be prevented.
>
> Best regards,
> Martin
>
>
> -----Original Message-----
> From: hotspot-dev <hotspot-dev-bounces at openjdk.java.net> On Behalf Of Gustavo Romero
> Sent: Montag, 3. September 2018 14:57
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Michihiro Horie <HORIE at jp.ibm.com>
> Cc: hotspot compiler <hotspot-compiler-dev at openjdk.java.net>; hotspot-dev at openjdk.java.net
> Subject: Re: RFR: 8208171: PPC64: Enrich SLP support
>
> Hi Goetz,
>
> On 09/03/2018 09:27 AM, Lindenmaier, Goetz wrote:
>> Also, I can not find all of the mail traffic in
>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-August/thread.html.
>> Is this a problem of the pipermail server?
>>
>> For some reason this webrev lacks the links to browse the diffs.
>> Do you need to use a more recent webrev? You can obtain it with
>> hg clone http://hg.openjdk.java.net/code-tools/webrev/ .
>
> Yes, probably it was a problem of the pipermail or in some relay.
> I noted the same thing, i.e. at least one Michi reply arrived
> to me but missed a ML.
>
> The initial discussion is here:
> http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2018-July/003613.html
>
> I understand Martin reviewed the last webrev in that thread, which is
> http://cr.openjdk.java.net/~mhorie/8208171/webrev.01/ (taken from
> http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2018-July/003615.html)
>
> Martin's review of webrev.01:
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-August/033958.html
>
> and Michi's reply to Martin's review of webrev.01:
> http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2018-August/003632.html (with webrev.02,
> taken from http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2018-August/003632.html).
>
> and your last review:
> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2018-September/030419.html
>
>
> HTH.
>
> Best regards,
> Gustavo
>
>> Why do you rename vnoreg to vnoregi?
>>
>> Besides that the change is fine, thanks for implementing this!
>>
>> Best regards,
>> Goetz.
>>
>>
>>> -----Original Message-----
>>> From: Doerr, Martin
>>> Sent: Dienstag, 28. August 2018 19:35
>>> To: Gustavo Romero <gromero at linux.vnet.ibm.com>; Michihiro Horie
>>> <HORIE at jp.ibm.com>
>>> Cc: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-
>>> dev at openjdk.java.net; ppc-aix-port-dev at openjdk.java.net; Simonis, Volker
>>> <volker.simonis at sap.com>
>>> Subject: RE: RFR: 8208171: PPC64: Enrich SLP support
>>>
>>> Hi Michihiro,
>>>
>>> thank you for implementing it. I have just taken a first look at your
>>> webrev.01.
>>>
>>> It looks basically good. Only the Power version check seems to be incorrect.
>>> VM_Version::has_popcntb() checks for Power5.
>>> I believe most instructions are available with Power7.
>>> Some ones (vsubudm, ..., vmmuluwm, vpopcntw) were introduced with
>>> Power8?
>>> We should check this carefully.
>>>
>>> Also, indentation in register_ppc.hpp could get improved.
>>>
>>> Thanks and best regard,
>>> Martin
>>>
>>>
>>> -----Original Message-----
>>> From: Gustavo Romero <gromero at linux.vnet.ibm.com>
>>> Sent: Donnerstag, 26. Juli 2018 16:02
>>> To: Michihiro Horie <HORIE at jp.ibm.com>
>>> Cc: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-
>>> dev at openjdk.java.net; Doerr, Martin <martin.doerr at sap.com>; ppc-aix-
>>> port-dev at openjdk.java.net; Simonis, Volker <volker.simonis at sap.com>
>>> Subject: Re: RFR: 8208171: PPC64: Enrich SLP support
>>>
>>> Hi Michi,
>>>
>>> On 07/26/2018 01:43 AM, Michihiro Horie wrote:
>>>> I updated webrev:
>>>> http://cr.openjdk.java.net/~mhorie/8208171/webrev.01/
>>>
>>> Thanks for providing an updated webrev and for fixing indentation and
>>> function
>>> order in assembler_ppc.inline.hpp as well. I have no further comments :)
>>>
>>>
>>> Best Regards,
>>> Gustavo
>>>
>>>>
>>>> Best regards,
>>>> --
>>>> Michihiro,
>>>> IBM Research - Tokyo
>>>>
>>>> Inactive hide details for Gustavo Romero ---2018/07/25 23:05:32---Hi Michi,
>>> On 07/25/2018 02:43 AM, Michihiro Horie wrote:Gustavo Romero ---
>>> 2018/07/25 23:05:32---Hi Michi, On 07/25/2018 02:43 AM, Michihiro Horie
>>> wrote:
>>>>
>>>> From: Gustavo Romero <gromero at linux.vnet.ibm.com>
>>>> To: Michihiro Horie/Japan/IBM at IBMJP, ppc-aix-port-
>>> dev at openjdk.java.net, hotspot-dev at openjdk.java.net
>>>> Cc: goetz.lindenmaier at sap.com, volker.simonis at sap.com, "Doerr, Martin"
>>> <martin.doerr at sap.com>
>>>> Date: 2018/07/25 23:05
>>>> Subject: Re: RFR: 8208171: PPC64: Enrich SLP support
>>>>
>>>> -------------------------------------------------------------------------------------------
>>> ----------------------------------------------------------------------------------------------
>>> ----------------------------------------------------------------------------------------------
>>> ----------------------------------------------------------------------------------------------
>>> ----------------------------------------------------------------------------------------------
>>> ----------------------------------------------------------------------------------------------
>>> ----------------------------------------------------------------------------------------------
>>> ----------------------------------------------------------------------------------------------
>>> ----------------------------------------------------------------------------------------------
>>> ----------------------------------------------------------------------------------------------
>>> -----------------------------------------------------
>>>>
>>>>
>>>>
>>>> Hi Michi,
>>>>
>>>> On 07/25/2018 02:43 AM, Michihiro Horie wrote:
>>>> > Dear all,
>>>> >
>>>> > Would you review the following change?
>>>> > Bug: https://bugs.openjdk.java.net/browse/JDK-8208171
>>>> > Webrev: http://cr.openjdk.java.net/~mhorie/8208171/webrev.00
>>>> >
>>>> > This change adds support for vectorized arithmetic calculation with SLP.
>>>> >
>>>> > The to_vr function is added to convert VSR to VR. Currently, vecX is
>>> associated with a VSR class vs_reg that only defines VSR32-51 in ppc.ad,
>>> which are exactly overlapped with VRs. Instruction APIs receiving VRs use the
>>> to_vr via vecX. Another thing is the change in sqrtF_reg to enable the
>>> matching with SqrtVF. I think the change in sqrtF_reg would be fine due to
>>> the ConvD2FNode::Value in convertnode.cpp.
>>>>
>>>> Looks good. Just a few comments:
>>>>
>>>> - In vmul4F_reg() would it be reasonable to use xvmulsp instead of
>>> vmaddfp in
>>>> order to avoid the splat?
>>>>
>>>> - Although all instructions added by your change where introduced in ISA
>>> 2.06,
>>>> so POWER7 and above are OK, as I see probes for
>>> PowerArchictecturePPC64=6|5 in
>>>> vm_version_ppc.cpp (line 64), I'm wondering if there is any control point
>>> to
>>>> guarantee that these instructions won't be emitted on a CPU that does
>>> not
>>>> support them.
>>>>
>>>> - I think that in general string in format %{} are in upper case. For instance,
>>>> this the current output on optoassembly for vmul4F:
>>>>
>>>> 2941835 5b4 ADDI R24, R24, #64
>>>> 2941836 5b8 vmaddfp VSR32,VSR32,VSR36 ! mul packed4F
>>>> 2941837 5c0 STXVD2X [R17], VSR32 // store 16-byte Vector
>>>>
>>>> I think it would be better to be in upper case instead. I also think that if
>>>> the node match emits more than one instruction all instructions must be
>>> listed
>>>> in format %{}, since it's meant for detailed debugging. Finally I think it
>>>> would be better to replace \t! by \t// in that string (unless I'm missing any
>>>> special meaning for that char). So for vmul4F it would be something like:
>>>>
>>>> 2941835 5b4 ADDI R24, R24, #64
>>>> VSPLTISW VSR34, 0 // Splat 0 imm in VSR34
>>>> 2941836 5b8 VMADDFP VSR32,VSR32,VSR36,VSR34 // Mul packed4F
>>>> 2941837 5c0 STXVD2X [R17], VSR32 // store 16-byte Vector
>>>>
>>>>
>>>> But feel free to change anything just after you get additional reviews :)
>>>>
>>>>
>>>> > I confirmed this change with JTREG. In addition, I used attached micro
>>> benchmarks.
>>>> > /(See attached file: slp_microbench.zip)/
>>>>
>>>> Thanks for sharing it.
>>>> Btw, another option to host it would be in the CR
>>>> server, in http://cr.openjdk.java.net/~mhorie/8208171
>>>>
>>>>
>>>> Best regards,
>>>> Gustavo
>>>>
>>>> >
>>>> > Best regards,
>>>> > --
>>>> > Michihiro,
>>>> > IBM Research - Tokyo
>>>> >
>>>>
>>>>
>>>>
>>
>
More information about the hotspot-compiler-dev
mailing list