[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-compiler-dev
Subject:    Re: [9] RFR(S): 8148490: RegisterSaver::restore_live_registers() fails to restore xmm registers on 3
From:       Vladimir Kozlov <vladimir.kozlov () oracle ! com>
Date:       2016-01-30 1:38:54
Message-ID: 56AC142E.6010309 () oracle ! com
[Download RAW message or body]

Michael,

Thank you for testing changes.
Please, file JBS bug for xml.transform problem.

Thanks,
Vladimir

On 1/29/16 2:28 PM, Berg, Michael C wrote:
> Tobias/Vladimir:
> 
> I would change the two asserts to in the 64bit code to make the check clear:
> 
> assert(UseAVX > 0, "up to 512bit vectors are supported with EVEX");
> assert(MaxVectorSize <= 64, "up to 512bit vectors are supported now");
> 
> As for testing with the patch applied to hotspot on a current jdk(01-29-16):
> 
> Windows sde 32-bit: skx - pass, also ran and passed part of specjvm2008
> Windows 32-bit: hsw - pass, also ran and passed all of specjvm2008
> Windows sde 64-bit: skx - pass, also ran and passed part of specjvm2008
> Windows 64-bit: hsw -pass, also ran and passed all of specjvm2008 : caveat
> Linux on skx: 32-bit - pass, also ran and passed all of specjvm2008
> Linux on skx:64-bit - pass, also ran and passed all of specjvm2008
> 
> We should proceed with checkin in the changelist after the usual testing.
> 
> Note: The above tests were done with the asserts changed on windows only. The 64bit \
> changes are mostly cosmetic.  It's the change to the additional_frame_bytes that \
> makes it correct, we used equivalent constants in the stack adjustment beforehand, \
> they had not been mapped to the movdqu for the non-vector case for a few iterations \
> on the file.  Early on I did have that code though. 
> Caveat: xml.transform fails with the changelist and without, I checked this against \
> a 12-21-15 built jdk which is 1 month old, so we have a new bug that is causing \
> this app to fail as well (on windows for 64bit) on hsw. I checked recent jbs \
> traffic, the occurrence does not appear to be tracked at this time. 
> -Michael
> 
> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov@oracle.com]
> Sent: Friday, January 29, 2016 11:40 AM
> To: hotspot-compiler-dev@openjdk.java.net
> Cc: Berg, Michael C
> Subject: Re: [9] RFR(S): 8148490: RegisterSaver::restore_live_registers() fails to \
> restore xmm registers on 32 bit 
> Tobias, please verify that 64-bit code works correctly.
> About 32-bit code.
> 
> Please verify correctness of next asserts:
> 
> assert(UseAVX > 0, "512bit vectors are supported only with EVEX");
> assert(MaxVectorSize == 64, "only 512bit vectors are supported now");
> 
> Originally we could have vectors even with only 64bit XMM registers. MaxVectorSize \
>                 and UseAVX can be set on command line
> - what happens in such case? No vectorization?
> 
> May be it is done because we save whole 128bit XMM always. Still MaxVectorSize == \
> 64 condition is strange. 
> Thanks,
> Vladimir
> 
> On 1/29/16 6:16 AM, Tobias Hartmann wrote:
> > Hi,
> > 
> > please review the following patch:
> > https://bugs.openjdk.java.net/browse/JDK-8148490
> > http://cr.openjdk.java.net/~thartmann/8148490/webrev.00/
> > 
> > RegisterSaver::save_live_registers() and RegisterSaver::restore_live_registers() \
> > are used by the safepoint handling code to save and restore registers. The \
> > following code is emitted to save and restore XMM/YMM registers on 32 bit: 
> > Save:
> > ...
> > 0xf34ca12e:	vmovdqu %xmm0,0xb0(%esp)
> > 0xf34ca137:	vmovdqu %xmm1,0xc0(%esp)
> > ...
> > 0xf34ca16d:	vmovdqu %xmm7,0x120(%esp)
> > 0xf34ca176:	sub    $0x80,%esp
> > 0xf34ca17c:	vextractf128 $0x1,%ymm0,(%esp)
> > 0xf34ca183:	vextractf128 $0x1,%ymm1,0x10(%esp)
> > ...
> > 0xf34ca1b3:	vextractf128 $0x1,%ymm7,0x70(%esp)
> > ...
> > 
> > Restore:
> > ...
> > 0xf34ca202:	vinsertf128 $0x1,(%esp),%ymm0,%ymm0
> > 0xf34ca209:	vinsertf128 $0x1,0x10(%esp),%ymm1,%ymm1
> > ...
> > 0xf34ca239:	vinsertf128 $0x1,0x70(%esp),%ymm7,%ymm7
> > 0xf34ca241:	add    $0x80,%esp
> > 0xf34ca247:	vmovdqu 0x130(%esp),%xmm0
> > 0xf34ca250:	vmovdqu 0x140(%esp),%xmm1
> > ...
> > 0xf34ca286:	vmovdqu 0x1a0(%esp),%xmm7
> > ...
> > 
> > The stack offsets for the vmovdqu instructions are wrong, causing the XMM \
> > registers to contain random values after a safepoint. The problem is that \
> > "additional_frame_bytes" is added to the stack offset although the stack pointer \
> > is incremented just before: 
> > 283     __ addptr(rsp, additional_frame_bytes); // Save upper half of YMM \
> > registers 
> > The regression test fails with "Test failed: array[0] = 1973.0 but should be \
> > 10.000" because the vectorized loop returns a wrong result. 
> > I spotted and fixed the following other problems:
> > - the vmovdqu instructions should be emitted before restoring YMM and ZMM because \
> >                 they zero the upper part of the XMM registers (i.e. YMM/ZMM)
> > - if 'UseAVX > 2' is set/available, we save the ZMM registers as well but we do \
> > not increment 'additional_frame_words' accordingly (we need another 8*32 bytes of \
> > stack space) 
> > Unfortunately, I don't have access to a CPU with the AVX-512 instruction set to \
> > test the "UseAVX > 2" related changes. Michael, could you verify the changes? 
> > The problems were introduced by the fix for JDK-8142980.
> > 
> > Thanks,
> > Tobias
> > 


[prev in list] [next in list] [prev in thread] [next in thread]