RFR(S): 8212481: PPC64: Enable POWER9 CPU detection

Wed Oct 31 14:34:48 UTC 2018

Hi Martin,

On 10/31/2018 11:09 AM, Doerr, Martin wrote:
> Hi Gustavo,
> 
> your copyright update looks correct. Ideally, both copyright lines should be in sync.
> But some people use scripts which only update Oracle's copyright. I believe this is why they sometimes get out of sync.

oh I see.

Thanks for clarifying and confirming the copyright update is correct.

I'll push the change with Volker's comments today.

Best regards,
Gustavo

> Best regards,
> Martin
> 
> 
> -----Original Message-----
> From: Gustavo Romero <gromero at linux.vnet.ibm.com>
> Sent: Mittwoch, 31. Oktober 2018 13:27
> To: Volker Simonis <volker.simonis at gmail.com>
> Cc: Doerr, Martin <martin.doerr at sap.com>; HotSpot Open Source Developers <hotspot-dev at openjdk.java.net>; ppc-aix-port-dev at openjdk.java.net
> Subject: Re: RFR(S): 8212481: PPC64: Enable POWER9 CPU detection
> 
> Hi Volker!
> 
> On 10/30/2018 02:55 PM, Volker Simonis wrote:
>> Hi Gustavo,
>>
>> thanks for adding Power9 support. Your change looks good!
> 
> Thanks a lot for reviewing.
> 
> 
>> Just two minor comments (no need to publish a new webrev):
>>
>> - can you please update the copyright year to 2018 in
>> assembler_ppc.inline.hpp and vm_version_ppc.hpp
> 
> I think that the copyright update applies to Oracle and SAP,
> like the following, but I would like to confirm if that is
> indeed correct as I see some examples "out of sync" in other
> files:
> 
> diff -r 57a87060bd09 src/hotspot/cpu/ppc/assembler_ppc.inline.hpp
> --- a/src/hotspot/cpu/ppc/assembler_ppc.inline.hpp      Tue Oct 16 16:26:28 2018 -0400
> +++ b/src/hotspot/cpu/ppc/assembler_ppc.inline.hpp      Wed Oct 31 08:03:21 2018 -0400
> @@ -1,6 +1,6 @@
>    /*
> - * Copyright (c) 2002, 2017, Oracle and/or its affiliates. All rights reserved.
> - * Copyright (c) 2012, 2017 SAP SE. All rights reserved.
> + * Copyright (c) 2002, 2018, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2012, 2018 SAP SE. All rights reserved.
>     * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>     *
>     * This code is free software; you can redistribute it and/or modify it
> diff -r 57a87060bd09 src/hotspot/cpu/ppc/vm_version_ppc.hpp
> --- a/src/hotspot/cpu/ppc/vm_version_ppc.hpp    Tue Oct 16 16:26:28 2018 -0400
> +++ b/src/hotspot/cpu/ppc/vm_version_ppc.hpp    Wed Oct 31 08:03:21 2018 -0400
> @@ -1,6 +1,6 @@
>    /*
> - * Copyright (c) 1997, 2017, Oracle and/or its affiliates. All rights reserved.
> - * Copyright (c) 2012, 2017 SAP SE. All rights reserved.
> + * Copyright (c) 1997, 2018, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2012, 2018 SAP SE. All rights reserved.
>     * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
>     *
>     * This code is free software; you can redistribute it and/or modify it
> 
> 
>> - in assembler_ppc.inline.hpp can you pleas add a comment with the
>> default value as shown below? I often find that useful when browsing
>> the code.
>>
>> +// Deliver A Random Number (introduced with POWER9)
>> +inline void Assembler::darn(Register d, int l /* = 1 */) {
>> emit_int32( DARN_OPCODE | rt(d) | l14(l)); }
>> +
> 
> Sure, I'll update that before pushing the change.
> 
> Thank you.
> 
> 
> Best regards,
> Gustavo
> 
>> Thank you and best regards,
>> Volker
>>
>> On Wed, Oct 24, 2018 at 3:35 PM Gustavo Romero
>> <gromero at linux.vnet.ibm.com> wrote:
>>>
>>> Hi Martin,
>>>
>>> On 10/24/2018 05:20 AM, Doerr, Martin wrote:
>>>> looks good. Thank you for cleaning things up and also for sharing the story about mftgpr. Interesting.
>>>> Reviewed.
>>>
>>> Thanks for reviewing!
>>>
>>>
>>> Best regards,
>>> Gustavo
>>>
>>>> Best regards,
>>>> Martin
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Gustavo Romero <gromero at linux.vnet.ibm.com>
>>>> Sent: Dienstag, 23. Oktober 2018 23:46
>>>> To: Doerr, Martin <martin.doerr at sap.com>; hotspot-dev at openjdk.java.net; ppc-aix-port-dev at openjdk.java.net
>>>> Subject: Re: RFR(S): 8212481: PPC64: Enable POWER9 CPU detection
>>>>
>>>> Hi Martin,
>>>>
>>>> Thanks for your comments on fsqrt{,s} and mftgpr opcodes.
>>>>
>>>> On 10/23/2018 01:51 PM, Doerr, Martin wrote:
>>>>> Hi Gustavo,
>>>>>
>>>>>> I agree. So, if you don't mind, to reduce any future rework on that
>>>>>> I removed the hardcoded L=1 and now it's just a default. I introduced
>>>>>> l14() function that matches two bits in the proper field as I don't
>>>>>> see any collision with any other one bit field at the same position
>>>>>> (bit 14).
>>>>> I like it, but the default value needs to get specified in the .hpp file.
>>>>
>>>> Fixed.
>>>>
>>>>
>>>>>> I just have one question regarding the feature-string. I see
>>>>>> instrumentation for "fsqrts" feature but it's not currently
>>>>>> advertised by the feature-string. On the other hand, ISA info
>>>>>> looks to indicate that fsqrt and fsqrts are not aliases, because:
>>>>>>
>>>>>> fsqrt  P2  -> Instruction introduced in the POWER2 Architecture.
>>>>>> fsqrts PPC -> Instruction introduced in the PowerPC Architecture prior to ISA v2.00
>>>>>>
>>>>>> so I'm wondering if just for completeness the "fsqrts" feature
>>>>>> should be included in the feature-string. Besides that I don't
>>>>>> see instrumentation to support has_mftgpr(), which is commented
>>>>>> out, thus should we remove it? Like the following:
>>>>>
>>>>> This code is very old. If we are sure nobody can run the VM on a machine without these instructions they can get removed from the feature checking.
>>>>
>>>> I'm not sure about it. Maybe somebody is using out there, in the community
>>>> for example, so kept it.
>>>>
>>>>
>>>>> The mftgpr was only designed for an extension of Power6 and the opcode was reused for something else later on if I remember correctly. I think you can remove the remaining comment.
>>>>
>>>> I was not aware of that, thanks for the info.
>>>> Just out of curiosity I asked the toolchain folks and they said that mftgpr
>>>> is present in some POWER6 hardware but on the so-called raw mode (ie power6x,
>>>> which I think it's the extension you mentioned), but not in the architected /
>>>> normal mode (power6). So even if it's present in the hardware it's disabled
>>>> unless you boot the system up in raw mode.
>>>>
>>>> On that raw mode (power6x) one would see the following in AUXV, for instance:
>>>>
>>>> $ LD_SHOW_AUXV=1 /bin/true | grep PLATFORM
>>>> AT_PLATFORM:     power6x
>>>> AT_BASE_PLATFORM:power6x
>>>>
>>>> But probably theses machines w/ power6x never got out of IBM indeed.
>>>>
>>>> Anyway, they said that power6x is unsupported. So, as you said, I removed the
>>>> comment about mftgpr.
>>>>
>>>> v3 webrev:
>>>>
>>>> http://cr.openjdk.java.net/~gromero/8212481/v3/
>>>>
>>>>
>>>> Thanks!
>>>>
>>>> Best regards,
>>>> Gustavo
>>>>
>>>
>>
> 