RFR(S): 8212481: PPC64: Enable POWER9 CPU detection
Hi, Could the following small change be reviewed please? Bug : https://bugs.openjdk.java.net/browse/JDK-8212481 Webrev: http://cr.openjdk.java.net/~gromero/8212481/v1/ It adds 'darn' instruction introduced with POWER9 CPUs to the Macroassembler and uses it to set PowerArchitecturePPC64 to POWER9 when the instruction is available on the CPU so it can be used in the future by any POWER9-specific JVM code or by any JVM code specifically dependent on the 'darn' instruction, using VM_Version::has_darn(). It was checked on POWER9 as the following: gromero@ltc-wspoon3:~/POWER9/jdk12_tip_f53671e05660+/jvm/openjdk-12-internal/bin$ lscpu Architecture: ppc64le Byte Order: Little Endian CPU(s): 176 On-line CPU(s) list: 0-175 Thread(s) per core: 4 Core(s) per socket: 22 Socket(s): 2 NUMA node(s): 8 Model: 2.2 (pvr 004e 1202) Model name: POWER9 (raw), altivec supported CPU max MHz: 2800.0000 CPU min MHz: 2300.0000 L1d cache: 32K L1i cache: 32K L2 cache: 512K L3 cache: 10240K NUMA node0 CPU(s): 0-87 NUMA node8 CPU(s): 88-175 NUMA node250 CPU(s): NUMA node251 CPU(s): NUMA node252 CPU(s): NUMA node253 CPU(s): NUMA node254 CPU(s): NUMA node255 CPU(s): gromero@ltc-wspoon3:~/POWER9/jdk12_tip_f53671e05660+/jvm/openjdk-12-internal/bin$ LD_SHOW_AUXV=1 /bin/true | egrep "HWCAP|PLATFORM" AT_HWCAP: true_le archpmu vsx arch_2_06 dfp ic_snoop smt mmu fpu altivec ppc64 ppc32 AT_HWCAP2: darn ieee128 arch_3_00 vcrypto tar isel ebb dscr arch_2_07 AT_PLATFORM: power9 AT_BASE_PLATFORM:power9 gromero@ltc-wspoon3:~/POWER9/jdk12_tip_f53671e05660+/jvm/openjdk-12-internal/bin$ ./java -XX:+Verbose -version dscr value was 0x10 Version: ppc64 fsqrt isel lxarxeh cmpb popcntb popcntw fcfids vand lqarx aes vpmsumb mfdscr vsx ldbrx stdbrx sha darn L1_data_cache_line_size=128 openjdk version "12-internal" 2019-03-19 OpenJDK Runtime Environment (fastdebug build 12-internal+0-f53671e05660.dirty.debug.POWER9) OpenJDK 64-Bit Server VM (fastdebug build 12-internal+0-f53671e05660.dirty.debug.POWER9, mixed mode, sharing) It was also checked on POWER8 and no regression was observed: gromero@gromero16:~/openjdks/jdk12_tip_f53671e05660+/jvm/openjdk-12-internal/bin$ lscpu Architecture: ppc64le Byte Order: Little Endian CPU(s): 80 On-line CPU(s) list: 0-79 Thread(s) per core: 8 Core(s) per socket: 10 Socket(s): 1 NUMA node(s): 1 Model: 2.1 (pvr 004b 0201) Model name: POWER8 (architected), altivec supported Hypervisor vendor: horizontal Virtualization type: full L1d cache: 64K L1i cache: 32K NUMA node0 CPU(s): 0-79 gromero@gromero16:~/openjdks/jdk12_tip_f53671e05660+/jvm/openjdk-12-internal/bin$ LD_SHOW_AUXV=1 /bin/true | egrep "HWCAP|PLATFORM" AT_HWCAP: archpmu vsx arch_2_06 dfp ic_snoop smt mmu fpu altivec ppc64 ppc32 AT_HWCAP2: htm-nosc vcrypto tar isel ebb dscr htm arch_2_07 AT_PLATFORM: power8 AT_BASE_PLATFORM:power8 gromero@gromero16:~/openjdks/jdk12_tip_f53671e05660+/jvm/openjdk-12-internal/bin$ ./java -XX:+Verbose -version dscr value was 0x0 Version: ppc64 fsqrt isel lxarxeh cmpb popcntb popcntw fcfids vand lqarx aes vpmsumb mfdscr vsx ldbrx stdbrx sha rtm L1_data_cache_line_size=128 openjdk version "12-internal" 2019-03-19 OpenJDK Runtime Environment (fastdebug build 12-internal+0-f53671e05660.dirty.debug.POWER9) OpenJDK 64-Bit Server VM (fastdebug build 12-internal+0-f53671e05660.dirty.debug.POWER9, mixed mode, sharing) Thank you. Best regards, Gustavo
Hi Gustavo, thanks for contributing. I have some minor remarks. I'm not sure if it's good to have L=1 hard coded in the assembler instruction, but I think it's ok for now. (One could also pass L as parameter with default value 1.) I think it would be nice to have the capability bit usages in the same order at all places in the source code. We could add a comment line like "rtm is determined by OS" where it was omitted. An empty line before has_mtfprd() would be good because it's just an alias. Having them everywhere as one block in the same order helps avoiding confusion. Besides this, the change looks good. Best regards, Martin -----Original Message----- From: ppc-aix-port-dev <ppc-aix-port-dev-bounces@openjdk.java.net> On Behalf Of Gustavo Romero Sent: Dienstag, 16. Oktober 2018 23:10 To: hotspot-dev@openjdk.java.net; ppc-aix-port-dev@openjdk.java.net Subject: RFR(S): 8212481: PPC64: Enable POWER9 CPU detection Hi, Could the following small change be reviewed please? Bug : https://bugs.openjdk.java.net/browse/JDK-8212481 Webrev: http://cr.openjdk.java.net/~gromero/8212481/v1/ It adds 'darn' instruction introduced with POWER9 CPUs to the Macroassembler and uses it to set PowerArchitecturePPC64 to POWER9 when the instruction is available on the CPU so it can be used in the future by any POWER9-specific JVM code or by any JVM code specifically dependent on the 'darn' instruction, using VM_Version::has_darn(). It was checked on POWER9 as the following: gromero@ltc-wspoon3:~/POWER9/jdk12_tip_f53671e05660+/jvm/openjdk-12-internal/bin$ lscpu Architecture: ppc64le Byte Order: Little Endian CPU(s): 176 On-line CPU(s) list: 0-175 Thread(s) per core: 4 Core(s) per socket: 22 Socket(s): 2 NUMA node(s): 8 Model: 2.2 (pvr 004e 1202) Model name: POWER9 (raw), altivec supported CPU max MHz: 2800.0000 CPU min MHz: 2300.0000 L1d cache: 32K L1i cache: 32K L2 cache: 512K L3 cache: 10240K NUMA node0 CPU(s): 0-87 NUMA node8 CPU(s): 88-175 NUMA node250 CPU(s): NUMA node251 CPU(s): NUMA node252 CPU(s): NUMA node253 CPU(s): NUMA node254 CPU(s): NUMA node255 CPU(s): gromero@ltc-wspoon3:~/POWER9/jdk12_tip_f53671e05660+/jvm/openjdk-12-internal/bin$ LD_SHOW_AUXV=1 /bin/true | egrep "HWCAP|PLATFORM" AT_HWCAP: true_le archpmu vsx arch_2_06 dfp ic_snoop smt mmu fpu altivec ppc64 ppc32 AT_HWCAP2: darn ieee128 arch_3_00 vcrypto tar isel ebb dscr arch_2_07 AT_PLATFORM: power9 AT_BASE_PLATFORM:power9 gromero@ltc-wspoon3:~/POWER9/jdk12_tip_f53671e05660+/jvm/openjdk-12-internal/bin$ ./java -XX:+Verbose -version dscr value was 0x10 Version: ppc64 fsqrt isel lxarxeh cmpb popcntb popcntw fcfids vand lqarx aes vpmsumb mfdscr vsx ldbrx stdbrx sha darn L1_data_cache_line_size=128 openjdk version "12-internal" 2019-03-19 OpenJDK Runtime Environment (fastdebug build 12-internal+0-f53671e05660.dirty.debug.POWER9) OpenJDK 64-Bit Server VM (fastdebug build 12-internal+0-f53671e05660.dirty.debug.POWER9, mixed mode, sharing) It was also checked on POWER8 and no regression was observed: gromero@gromero16:~/openjdks/jdk12_tip_f53671e05660+/jvm/openjdk-12-internal/bin$ lscpu Architecture: ppc64le Byte Order: Little Endian CPU(s): 80 On-line CPU(s) list: 0-79 Thread(s) per core: 8 Core(s) per socket: 10 Socket(s): 1 NUMA node(s): 1 Model: 2.1 (pvr 004b 0201) Model name: POWER8 (architected), altivec supported Hypervisor vendor: horizontal Virtualization type: full L1d cache: 64K L1i cache: 32K NUMA node0 CPU(s): 0-79 gromero@gromero16:~/openjdks/jdk12_tip_f53671e05660+/jvm/openjdk-12-internal/bin$ LD_SHOW_AUXV=1 /bin/true | egrep "HWCAP|PLATFORM" AT_HWCAP: archpmu vsx arch_2_06 dfp ic_snoop smt mmu fpu altivec ppc64 ppc32 AT_HWCAP2: htm-nosc vcrypto tar isel ebb dscr htm arch_2_07 AT_PLATFORM: power8 AT_BASE_PLATFORM:power8 gromero@gromero16:~/openjdks/jdk12_tip_f53671e05660+/jvm/openjdk-12-internal/bin$ ./java -XX:+Verbose -version dscr value was 0x0 Version: ppc64 fsqrt isel lxarxeh cmpb popcntb popcntw fcfids vand lqarx aes vpmsumb mfdscr vsx ldbrx stdbrx sha rtm L1_data_cache_line_size=128 openjdk version "12-internal" 2019-03-19 OpenJDK Runtime Environment (fastdebug build 12-internal+0-f53671e05660.dirty.debug.POWER9) OpenJDK 64-Bit Server VM (fastdebug build 12-internal+0-f53671e05660.dirty.debug.POWER9, mixed mode, sharing) Thank you. Best regards, Gustavo
Hi Martin, On 10/22/2018 07:07 AM, Doerr, Martin wrote:
Hi Gustavo,
thanks for contributing. I have some minor remarks. Thanks a lot for reviewing it.
I'm not sure if it's good to have L=1 hard coded in the assembler instruction, but I think it's ok for now. (One could also pass L as parameter with default value 1.)
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 think it would be nice to have the capability bit usages in the same order at all places in the source code. We could add a comment line like "rtm is determined by OS" where it was omitted. An empty line before has_mtfprd() would be good because it's just an alias. Having them everywhere as one block in the same order helps avoiding confusion.
Sure. I think that the confusion came from the fact that I split the "CPU instruction support" section in two by adding a new one called "OS instruction support" when I refactored the TM detection. Given a second thought tho I think it's not necessary, so I've put has_tm() back to the block as you instructed, adding an empty line before has_mtfprd() as it's an alias, and added the "rtm is deter- mined by OS" comment were it applies. v2 webrev: http://cr.openjdk.java.net/~gromero/8212481/v2/ 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: diff -r c860b03741ab src/hotspot/cpu/ppc/vm_version_ppc.cpp --- a/src/hotspot/cpu/ppc/vm_version_ppc.cpp Tue Oct 16 16:26:28 2018 -0400 +++ b/src/hotspot/cpu/ppc/vm_version_ppc.cpp Tue Oct 23 10:51:22 2018 -0400 @@ -134,12 +134,12 @@ // Create and print feature-string. char buf[(num_features+1) * 16]; // Max 16 chars per feature. jio_snprintf(buf, sizeof(buf), - "ppc64%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s", + "ppc64%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s", (has_fsqrt() ? " fsqrt" : ""), + (has_fsqrts() ? " fsqrts" : ""), (has_isel() ? " isel" : ""), (has_lxarxeh() ? " lxarxeh" : ""), (has_cmpb() ? " cmpb" : ""), - //(has_mftgpr()? " mftgpr" : ""), (has_popcntb() ? " popcntb" : ""), (has_popcntw() ? " popcntw" : ""), (has_fcfids() ? " fcfids" : ""), Thank you. Best regards, Gustavo
Besides this, the change looks good.
Best regards, Martin
-----Original Message----- From: ppc-aix-port-dev <ppc-aix-port-dev-bounces@openjdk.java.net> On Behalf Of Gustavo Romero Sent: Dienstag, 16. Oktober 2018 23:10 To: hotspot-dev@openjdk.java.net; ppc-aix-port-dev@openjdk.java.net Subject: RFR(S): 8212481: PPC64: Enable POWER9 CPU detection
Hi,
Could the following small change be reviewed please?
Bug : https://bugs.openjdk.java.net/browse/JDK-8212481 Webrev: http://cr.openjdk.java.net/~gromero/8212481/v1/
It adds 'darn' instruction introduced with POWER9 CPUs to the Macroassembler and uses it to set PowerArchitecturePPC64 to POWER9 when the instruction is available on the CPU so it can be used in the future by any POWER9-specific JVM code or by any JVM code specifically dependent on the 'darn' instruction, using VM_Version::has_darn().
It was checked on POWER9 as the following:
gromero@ltc-wspoon3:~/POWER9/jdk12_tip_f53671e05660+/jvm/openjdk-12-internal/bin$ lscpu Architecture: ppc64le Byte Order: Little Endian CPU(s): 176 On-line CPU(s) list: 0-175 Thread(s) per core: 4 Core(s) per socket: 22 Socket(s): 2 NUMA node(s): 8 Model: 2.2 (pvr 004e 1202) Model name: POWER9 (raw), altivec supported CPU max MHz: 2800.0000 CPU min MHz: 2300.0000 L1d cache: 32K L1i cache: 32K L2 cache: 512K L3 cache: 10240K NUMA node0 CPU(s): 0-87 NUMA node8 CPU(s): 88-175 NUMA node250 CPU(s): NUMA node251 CPU(s): NUMA node252 CPU(s): NUMA node253 CPU(s): NUMA node254 CPU(s): NUMA node255 CPU(s): gromero@ltc-wspoon3:~/POWER9/jdk12_tip_f53671e05660+/jvm/openjdk-12-internal/bin$ LD_SHOW_AUXV=1 /bin/true | egrep "HWCAP|PLATFORM" AT_HWCAP: true_le archpmu vsx arch_2_06 dfp ic_snoop smt mmu fpu altivec ppc64 ppc32 AT_HWCAP2: darn ieee128 arch_3_00 vcrypto tar isel ebb dscr arch_2_07 AT_PLATFORM: power9 AT_BASE_PLATFORM:power9 gromero@ltc-wspoon3:~/POWER9/jdk12_tip_f53671e05660+/jvm/openjdk-12-internal/bin$ ./java -XX:+Verbose -version dscr value was 0x10 Version: ppc64 fsqrt isel lxarxeh cmpb popcntb popcntw fcfids vand lqarx aes vpmsumb mfdscr vsx ldbrx stdbrx sha darn L1_data_cache_line_size=128
openjdk version "12-internal" 2019-03-19 OpenJDK Runtime Environment (fastdebug build 12-internal+0-f53671e05660.dirty.debug.POWER9) OpenJDK 64-Bit Server VM (fastdebug build 12-internal+0-f53671e05660.dirty.debug.POWER9, mixed mode, sharing)
It was also checked on POWER8 and no regression was observed:
gromero@gromero16:~/openjdks/jdk12_tip_f53671e05660+/jvm/openjdk-12-internal/bin$ lscpu Architecture: ppc64le Byte Order: Little Endian CPU(s): 80 On-line CPU(s) list: 0-79 Thread(s) per core: 8 Core(s) per socket: 10 Socket(s): 1 NUMA node(s): 1 Model: 2.1 (pvr 004b 0201) Model name: POWER8 (architected), altivec supported Hypervisor vendor: horizontal Virtualization type: full L1d cache: 64K L1i cache: 32K NUMA node0 CPU(s): 0-79 gromero@gromero16:~/openjdks/jdk12_tip_f53671e05660+/jvm/openjdk-12-internal/bin$ LD_SHOW_AUXV=1 /bin/true | egrep "HWCAP|PLATFORM" AT_HWCAP: archpmu vsx arch_2_06 dfp ic_snoop smt mmu fpu altivec ppc64 ppc32 AT_HWCAP2: htm-nosc vcrypto tar isel ebb dscr htm arch_2_07 AT_PLATFORM: power8 AT_BASE_PLATFORM:power8 gromero@gromero16:~/openjdks/jdk12_tip_f53671e05660+/jvm/openjdk-12-internal/bin$ ./java -XX:+Verbose -version dscr value was 0x0 Version: ppc64 fsqrt isel lxarxeh cmpb popcntb popcntw fcfids vand lqarx aes vpmsumb mfdscr vsx ldbrx stdbrx sha rtm L1_data_cache_line_size=128
openjdk version "12-internal" 2019-03-19 OpenJDK Runtime Environment (fastdebug build 12-internal+0-f53671e05660.dirty.debug.POWER9) OpenJDK 64-Bit Server VM (fastdebug build 12-internal+0-f53671e05660.dirty.debug.POWER9, mixed mode, sharing)
Thank you.
Best regards, Gustavo
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.
Sure. I think that the confusion came from the fact that I split the "CPU instruction support" section in two by adding a new one called "OS instruction support" when I refactored the TM detection. Given a second thought tho I think it's not necessary, so I've put has_tm() back to the block as you instructed, adding an empty line before has_mtfprd() as it's an alias, and added the "rtm is deter- mined by OS" comment were it applies. Thanks for cleaning it up.
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. 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. Best regards, Martin
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
Hi Gustavo, looks good. Thank you for cleaning things up and also for sharing the story about mftgpr. Interesting. Reviewed. Best regards, Martin -----Original Message----- From: Gustavo Romero <gromero@linux.vnet.ibm.com> Sent: Dienstag, 23. Oktober 2018 23:46 To: Doerr, Martin <martin.doerr@sap.com>; hotspot-dev@openjdk.java.net; ppc-aix-port-dev@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
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@linux.vnet.ibm.com> Sent: Dienstag, 23. Oktober 2018 23:46 To: Doerr, Martin <martin.doerr@sap.com>; hotspot-dev@openjdk.java.net; ppc-aix-port-dev@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
Hi Gustavo, thanks for adding Power9 support. Your change looks good! 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 - 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)); } + Thank you and best regards, Volker On Wed, Oct 24, 2018 at 3:35 PM Gustavo Romero <gromero@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@linux.vnet.ibm.com> Sent: Dienstag, 23. Oktober 2018 23:46 To: Doerr, Martin <martin.doerr@sap.com>; hotspot-dev@openjdk.java.net; ppc-aix-port-dev@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
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@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@linux.vnet.ibm.com> Sent: Dienstag, 23. Oktober 2018 23:46 To: Doerr, Martin <martin.doerr@sap.com>; hotspot-dev@openjdk.java.net; ppc-aix-port-dev@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
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@linux.vnet.ibm.com> Sent: Mittwoch, 31. Oktober 2018 13:27 To: Volker Simonis <volker.simonis@gmail.com> Cc: Doerr, Martin <martin.doerr@sap.com>; HotSpot Open Source Developers <hotspot-dev@openjdk.java.net>; ppc-aix-port-dev@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@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@linux.vnet.ibm.com> Sent: Dienstag, 23. Oktober 2018 23:46 To: Doerr, Martin <martin.doerr@sap.com>; hotspot-dev@openjdk.java.net; ppc-aix-port-dev@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
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@linux.vnet.ibm.com> Sent: Mittwoch, 31. Oktober 2018 13:27 To: Volker Simonis <volker.simonis@gmail.com> Cc: Doerr, Martin <martin.doerr@sap.com>; HotSpot Open Source Developers <hotspot-dev@openjdk.java.net>; ppc-aix-port-dev@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@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@linux.vnet.ibm.com> Sent: Dienstag, 23. Oktober 2018 23:46 To: Doerr, Martin <martin.doerr@sap.com>; hotspot-dev@openjdk.java.net; ppc-aix-port-dev@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
participants (3)
-
Doerr, Martin
-
Gustavo Romero
-
Volker Simonis