RFR(M): 8234599: PPC64: Add support on recent CPUs and Linux for JEP-352

[Doerr, Martin]

Hi Gustavo,

thanks for the update. Looks good.

Please remove the whitespaces between the instructions and '(' in generate_data_cache_writeback_sync() before pushing.
Marked here by 'X':
+     __ andi_X(temp, is_presync, 1);
+     __ bneX(CCR0, SKIP);
+     __ cache_wbsync(false); // post sync => emit 'sync'
+     __ bindX(SKIP);         // pre sync => emit nothing

Best regards,
Martin


> -----Original Message-----
> From: Gustavo Romero <gromero at linux.vnet.ibm.com>
> Sent: Mittwoch, 18. Dezember 2019 16:46
> To: Doerr, Martin <martin.doerr at sap.com>; Baesken, Matthias
> <matthias.baesken at sap.com>
> Cc: Andrew Dinn <adinn at redhat.com>; hotspot-compiler-
> dev at openjdk.java.net
> Subject: Re: RFR(M): 8234599: PPC64: Add support on recent CPUs and Linux
> for JEP-352
> 
> Hi Martin,
> 
> On 12/11/2019 01:55 PM, Doerr, Martin wrote:
> > Hi Gustavo,
> >
> > thanks for implementing it. Unfortunately, we can't test it at the moment.
> 
> Thanks a lot for the review.
> 
> 
> > I have a few change requests:
> >
> >
> > macroAssembler_ppc.cpp
> > I don't like silently emitting nothing in case
> !VM_Version::supports_data_cache_line_flush().
> > If you want to check for it, I suggest to assert
> VM_Version::supports_data_cache_line_flush() and avoid generating the
> stub otherwise (stubGenerator_ppc).
> 
> Fixed.
> 
> 
> >
> > ppc.ad
> > The predicates are redundant and should better get removed (useless
> evaluation).
> 
> oh ... Fixed.
> 
> 
> > cacheWBPreSync could use cost 0 for clearity. (The costs don't have any
> effect because there is no choice for the matcher.)
> 
> Fixed.
> 
> 
> > stubGenerator_ppc.cpp
> > I think checking cmpwi(... is_presync, 1) is ok because the ABI specifies that
> "bool true" is one Byte with the value 1 and the C calling convention enforces
> extension to 8 Byte.
> > I would have used andi_ + bne to be on the safe side, but I believe your
> version is ok.
> 
> I decided for the safe side as you suggested :)
> 
> 
> > Comment "// post sync => emit 'lwsync'" is wrong. We use 'sync'.
> 
> Sorry, it was a "thinko" when placing the comment. Indeed, the comment is
> wrong
> and the code is correct. Fixed.
> 
> I've also fixed the assert() compilation error on fastdebug accordingly to
> Matthias' comments.
> 
> Finally I tweaked a bit the 'format' strings in ppc.add to show a better output
> on +PrintAssembly. For instance, previously it would print something like:
> 
> 090     B7: #   out( B7 B8 ) <- in( B6 B7 ) Loop( B7-B7 inner ) Freq: 3.99733
> 090     MR      R17, R15        // Long->Ptr
> 094     cache wb [R17]
> 
> for the cache writeback. Now:
> 
> 094     cache writeback, address = [R17]
> 
> 
> Please find v2 at:
> 
> http://cr.openjdk.java.net/~gromero/8234599/v2/
> 
> 
> Best regards,
> Gustavo