[8u] RFR for backport of 8198894 (CRC32 1/4): [PPC64] More generic vector CRC implementation (v2)

[Doerr, Martin]

Hi Gustavo,

> [PPC64] More generic vector CRC implementation                           (1/4)
> http://cr.openjdk.java.net/~gromero/crc32_jdk8u/for-review/v2/8198894/

This seems to be the version I had already reviewed. I'm still ok with it.


> [PPC64] Possibly unreliable stack frame resizing in template interpreter (2/4)
> http://cr.openjdk.java.net/~gromero/crc32_jdk8u/for-review/v2/8216376/

Normally, backports should get handled in separate backport RFRs, but the manual change in this version could be considered trivial, so I don't insist on separate RFR.
Looks good, now.


> [PPC64] Vector CRC implementation should be used by interpreter and be faster for short arrays
> http://cr.openjdk.java.net/~gromero/crc32_jdk8u/for-review/v2/8216060/   (3/4)

Almost like above. I'd leave at least the CRC32C defines in the code (stubRoutines_ppc). They don't disturb. Why should we introduce additional diffs?


> [PPC64] Cleanup non-vector version of CRC32
> http://cr.openjdk.java.net/~gromero/crc32_jdk8u/for-review/v2/8217459/   (4/4)

This one contains a part of another shared code change. So I can't review it as part of this RFR.
Needs to get reviewed separately. Backport of the original change which introduced LITTLE_ENDIAN_ONLY etc. should get evaluated.


Best regards,
Martin


> -----Original Message-----
> From: Gustavo Romero <gromero at linux.vnet.ibm.com>
> Sent: Montag, 4. November 2019 23:33
> To: Doerr, Martin <martin.doerr at sap.com>; hotspot-compiler-
> dev at openjdk.java.net
> Cc: jdk8u-dev at openjdk.java.net
> Subject: Re: [8u] RFR for backport of 8198894 (CRC32 1/4): [PPC64] More
> generic vector CRC implementation (v2)
> 
> Hello Martin,
> 
> On 10/24/2019 07:17 AM, Doerr, Martin wrote:
> > Hi Gustavo,
> >
> > I think removing invertCRC is an unnecessary manual change.
> > We should minimize that as far as possible. They may create merge
> conflicts for future backports.
> 
> Thanks a lot for the review.
> 
> I agree I should minimize the changes as far as possible. I added back
> invertCRC
> and tried to follow your advice, so the final clean-up patch is almost similar
> to the one found on jdk/jdk, for instance.
> 
> Please find v2 for the patchset below. v2 changes affect only 3/4 and 4/4.
> 
> 
> [PPC64] More generic vector CRC implementation                           (1/4)
> http://cr.openjdk.java.net/~gromero/crc32_jdk8u/for-review/v2/8198894/
> 
> v2:
> - Adapt file names to OpenJDK 8u
> - Remove CRC32C part, leaving only CRC32 part, since OpenJDK 8u has no
> CRC32C
> - Add Assembler::add_const_optimized() from "8077838: Recent
> developments for ppc" [0]
> - Fix vpermxor() opcode, replacing VPMSUMW_OPCODE by
> VPERMXOR_OPCODE,
>    accordingly to fix in "8190781: ppc64 + s390: Fix CriticalJNINatives" [1]
> - Adapt signatures for the following functions and their callers, accordingly to
>    "8175369: [ppc] Provide intrinsic implementation for CRC32C" [2]:
>     a. MacroAssembler::update_byteLoop_crc32(), removing 'invertCRC'
> parameter
>     b. MacroAssembler::kernel_crc32_1word(), adding 'invertCRC' parameter
> 
> 
> [PPC64] Possibly unreliable stack frame resizing in template interpreter (2/4)
> http://cr.openjdk.java.net/~gromero/crc32_jdk8u/for-review/v2/8216376/
> 
> v2:
> - Adapt file names to OpenJDK 8u
> - Remove CRC32C code
> 
> 
> [PPC64] Vector CRC implementation should be used by interpreter and be
> faster for short arrays
> http://cr.openjdk.java.net/~gromero/crc32_jdk8u/for-review/v2/8216060/
> (3/4)
> 
> v2:
> - Remove CRC32C code, keeping is_crc32c in crc32(), code related to is_crc32c
>    and invertCRC, like code in kernel_crc32_vpmsum(), and not touching stub
> code
>    mark in generate_CRC32_updateBytes() to avoid merge conflicts in future
>    backports.
> 
> 
> [PPC64] Cleanup non-vector version of CRC32
> http://cr.openjdk.java.net/~gromero/crc32_jdk8u/for-review/v2/8217459/
> (4/4)
> 
> v2:
> - Add {BIG,LITTLE}_ENDIAN_ONLY to src/share/vm/utilities/macros.hpp
> - Add kernel_crc32_singleByteReg from change 8175369 [2] as the clean-up
> uses it
>    in InterpreterGenerator::generate_CRC32_update_entry().
> 
> 
> --
> 
> Best regards,
> Gustavo
> 
> [0] http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/88847a1b3718
> [1] http://hg.openjdk.java.net/jdk/jdk/rev/5a69ba3a4fd1#l1.7
> [2] https://bugs.openjdk.java.net/browse/JDK-8175369