RFR(M) JDK-8181809 PPC64: Leverage mtfprd/mffprd on POWER8

Tue Jul 4 13:19:57 UTC 2017

Hi Matthew, Martin,

thanks a lot for doing/reviewing this change!
It looks good. Thumbs up from my side.

Regards,
Volker

On Thu, Jun 29, 2017 at 3:37 PM, Matthew Brandyberry
<mbrandy at linux.vnet.ibm.com> wrote:
> Thanks Martin.  That looks good.  Is there anything I need to do to request
> a 2nd review?
>
>
> On 6/26/17 7:47 AM, Doerr, Martin wrote:
>>
>> Hi Matt,
>>
>> after some testing and reviewing the C1 part again, I found 2 bugs:
>>
>> c1_LIRAssembler: is_stack() can't be used for this purpose as the value
>> may be available in a register even though it was forced to stack. I just
>> changed src_in_memory = !VM_Version::has_mtfprd() to make it consistent with
>> LIRGenerator and removed the assertions which have become redundant.
>>
>> c1_LIRGenerator: value.set_destroys_register() is still needed for
>> conversion from FP to GP registers because they kill the src value by
>> fctiwz/fctidz.
>>
>> I just fixed these issues here in a copy of your webrev v2:
>> http://cr.openjdk.java.net/~mdoerr/8181809_ppc64_mtfprd/v2/
>>
>> Please take a look and use this one for 2nd review.
>>
>> Best regards,
>> Martin
>>
>>
>> -----Original Message-----
>> From: Doerr, Martin
>> Sent: Montag, 26. Juni 2017 10:44
>> To: 'Matthew Brandyberry' <mbrandy at linux.vnet.ibm.com>;
>> ppc-aix-port-dev at openjdk.java.net; hotspot-dev at openjdk.java.net
>> Subject: RE: RFR(M) JDK-8181809 PPC64: Leverage mtfprd/mffprd on POWER8
>>
>> Hi Matt,
>>
>> you can run the pre-push check stand-alone: hg jcheck
>> See:
>> http://openjdk.java.net/projects/code-tools/jcheck/
>>
>> I just had to add the commit message:
>> 8181809: PPC64: Leverage mtfprd/mffprd on POWER8
>> Reviewed-by: mdoerr
>> Contributed-by: Matthew Brandyberry <mbrandy at linux.vnet.ibm.com>
>>
>> (Note that the ':' after the bug id is important.)
>>
>> and replace the Tabs the 2 C1 files to get it passing.
>> (I think that "Illegal tag name" warnings can be ignored.)
>>
>> So only the copyright dates are missing which are not checked by jcheck.
>> But I don't need a new webrev if that's all which needs to be changed.
>>
>> Best regards,
>> Martin
>>
>>
>> -----Original Message-----
>> From: Matthew Brandyberry [mailto:mbrandy at linux.vnet.ibm.com]
>> Sent: Freitag, 23. Juni 2017 18:39
>> To: Doerr, Martin <martin.doerr at sap.com>;
>> ppc-aix-port-dev at openjdk.java.net; hotspot-dev at openjdk.java.net
>> Subject: Re: RFR(M) JDK-8181809 PPC64: Leverage mtfprd/mffprd on POWER8
>>
>> Thanks Martin. Are there tools to help detect formatting errors like the
>> tab characters?
>>
>> I'll keep an eye on this to see if I need to do anything else.
>>
>> -Matt
>>
>> On 6/23/17 4:30 AM, Doerr, Martin wrote:
>>>
>>> Excellent. Thanks for the update. The C1 part looks good, too.
>>>
>>> Also, thanks for checking "I could not find evidence of any config that
>>> includes vpmsumb but not
>>> mtfprd."
>>>
>>> There are only a few formally required things:
>>> - The new C1 code contains Tab characters. It's not possible to push it
>>> without fixing this.
>>> - Copyright messages should be updated.
>>> - Minor resolution to get vm_version_ppc applied to recent jdk10/hs.
>>>
>>> If no other changes get requested, I can handle these issues this time
>>> before pushing.
>>> But we need another review, first.
>>>
>>> Thanks and best regards,
>>> Martin
>>>
>>>
>>> -----Original Message-----
>>> From: Matthew Brandyberry [mailto:mbrandy at linux.vnet.ibm.com]
>>> Sent: Freitag, 23. Juni 2017 04:54
>>> To: Doerr, Martin <martin.doerr at sap.com>;
>>> ppc-aix-port-dev at openjdk.java.net; hotspot-dev at openjdk.java.net
>>> Subject: Re: RFR(M) JDK-8181809 PPC64: Leverage mtfprd/mffprd on POWER8
>>>
>>> Updated webrev: http://cr.openjdk.java.net/~gromero/8181809/v2/
>>>
>>> See below for responses inline.
>>>
>>> On 6/20/17 8:38 AM, Matthew Brandyberry wrote:
>>>>
>>>> Hi Martin,
>>>>
>>>> Thanks for the review.  I'll take a look at these areas and report
>>>> back -- especially the integration into C1.
>>>>
>>>> On 6/20/17 8:33 AM, Doerr, Martin wrote:
>>>>>
>>>>> Hi Matt,
>>>>>
>>>>> thanks for providing this webrev. I had already thought about using
>>>>> these instructions for this purpose and your change matches pretty
>>>>> much what I'd do.
>>>>>
>>>>> Here a couple of comments:
>>>>> ppc.ad:
>>>>> This was a lot of work. Thanks for doing it.
>>>>> effect(DEF dst, USE src); is redundant if a match rule match(Set dst
>>>>> (MoveL2D src)); exists.
>>>
>>> Fixed.
>>>>>
>>>>> vm_version:
>>>>> This part is in conflict with Michihiro's change which is already
>>>>> pushed in jdk10, but it's trivial to resolve. I'm ok with using
>>>>> has_vpmsumb() for has_mtfprd(). In the past, we sometimes had trouble
>>>>> with assuming that a certain Power processor supports all new
>>>>> instructions if it supports certain ones. We also use the hotspot
>>>>> code on as400 where certain instruction subsets were disabled while
>>>>> other Power 8 instructions were usable. Maybe you can double-check if
>>>>> there may exist configurations in which has_vpmsumb() doesn't match
>>>>> has_mtfprd().
>>>
>>> I could not find evidence of any config that includes vpmsumb but not
>>> mtfprd.
>>>>>
>>>>> C1:
>>>>> It should also be possible to use the instructions in C1 compiler.
>>>>> Maybe you would like to take a look at it as well and see if it can
>>>>> be done with feasible effort.
>>>>> Here are some hints:
>>>>> The basic decisions are made in LIRGenerator::do_Convert. You could
>>>>> skip the force_to_spill or must_start_in_memory steps.
>>>>> The final assembly code gets emitted in LIR_Assembler::emit_opConvert
>>>>> where you could replace the store instructions.
>>>>> For testing, you can use -XX:TieredStopAtLevel=1, for example.
>>>
>>> Done.  Please take a look.
>>>>>
>>>>> Thanks and best regards,
>>>>> Martin
>>>>>
>>>>>
>>>>> -----Original Message-----
>>>>> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net] On
>>>>> Behalf Of Matthew Brandyberry
>>>>> Sent: Montag, 19. Juni 2017 18:28
>>>>> To: ppc-aix-port-dev at openjdk.java.net; hotspot-dev at openjdk.java.net
>>>>> Subject: RFR(M) JDK-8181809 PPC64: Leverage mtfprd/mffprd on POWER8
>>>>>
>>>>> This is a PPC-specific hotspot optimization that leverages the
>>>>> mtfprd/mffprd instructions for for movement between general purpose and
>>>>> floating point registers (rather than through memory).  It yields a
>>>>> ~35%
>>>>> improvement measured via a microbenchmark. Please review: Bug:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8181809 Webrev:
>>>>> http://cr.openjdk.java.net/~gromero/8181809/v1/ Thanks, Matt
>>>>>
>>>>>
>