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

Wed Oct 31 14:09:07 UTC 2018

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.

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
>>>
>>
> 

