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

[Andrew Hughes]

On 27/09/2019 04:31, Gustavo Romero wrote:
> Hi,
> 
> Could the following backport for jdk8u be reviewed please?
> 
> The original change was mainly written to provide a better
> implementation for
> CRC32 and CRC32C, but it also improved the CRC32 performance in general.
> This
> backport is the first change of a patchset comprised of 4 changes aimed to
> backport all the latest CRC32 missing parts from JDK 11u.
> 
> It's a PPC64-only change.
> 
> Bug     : https://bugs.openjdk.java.net/browse/JDK-8198894
> Original: http://hg.openjdk.java.net/jdk/jdk/rev/7cd503c499a0
> Backport:
> http://cr.openjdk.java.net/~gromero/crc32_jdk8u/for-review/8198894/
> 

Can we please stick to one change per webrev, please? It makes it
confusing as to what will actually be pushed when a webrev refers to
multiple bugs which are not part of the RFR.

> It was necessary to backport it to:
> 
> - Adapt the file names to OpenJDK 8u

Fine, and doesn't warrant review on its own.

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

Cherry-picking this seems fine as we don't want to force POWER 8 as a
requirement on 8u.

> - Fix vpermxor() opcode, replacing VPMSUMW_OPCODE by VPERMXOR_OPCODE,
>   accordingly to fix in "8190781: ppc64 + s390: Fix CriticalJNINatives" [1]

It seems this fix should be backported as a pre-requisite. Was there a
reason not to? It seems a fairly simple change from a naive perspective.

> - Adapt signatures for the following functions and their callers,
> accordingly to
>   "8175369: [ppc] Provide intrinsic implementation for CRC32C" [2], also
> to ease
>   subsequent backports in this patchset:
>    a. MacroAssembler::update_byteLoop_crc32(), removing 'invertCRC'
> parameter
>    b. MacroAssembler::kernel_crc32_1word(), adding 'invertCRC' parameter
> 

I share Martin's concerns about this. If we must do this, I would prefer
it was done as a separate change. It seems unrelated to the other
changes in 8198894.

A few minor issues:

* The diff of the macroAssembler_ppc.cpp changes looks different from
the 11u version, though there doesn't seem to be any actual difference.
I don't know if it's possible to line this up better by checking no
empty lines were missed.

* macroAssembler_ppc.cpp, macroAssembler_ppc.hpp, stubGenerator_ppc.hpp,
stubRoutines_ppc_64.hpp & stubRoutines_ppc_64.cpp include copyright
header changes in the 11u patch which don't appear to be in the 8u
version. Were these already present or simply missed?

> 
> Thank you.
> 
> 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

Thanks,
-- 
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net)
Fingerprint = 5132 579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222