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

[Andrew Hughes]

On 25/02/2020 22:07, Gustavo Romero wrote:
> Hello Andrew,
> 
> Thanks for the review.
> 
> 
> On 02/18/2020 07:29 AM, Andrew Hughes wrote:
>> 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.
> 
> Yes, I know. I started the original request with one RFR per change, but
> then I
> messed up when I replied Martin the first time and put all the patches on a
> single RFR related to patch 1/4. Sorry about that.
> 
> So, just to be clear I started with the following RFRs:
> 
> [1] [8u] RFR for backport of 8198894 (CRC32 1/4): [PPC64] More generic
> vector CRC implementation
> [2] [8u] RFR for backport of 8216376 (CRC32 2/4): [PPC64] Possibly
> unreliable stack frame resizing in template interpreter
> [3] [8u] RFR for backport of 8216060 (CRC32 3/4): [PPC64] Vector CRC
> implementation should be used by interpreter and be faster for short arrays
> [4] [8u] RFR for backport of 8217459 (CRC32 4/4): [PPC64] Cleanup
> non-vector version of CRC32
> 
> [1]
> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-September/035218.html
> 
> [2]
> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-September/035219.html
> 
> [3]
> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-September/035221.html
> 
> [4]
> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-September/035220.html
> 
> 
> then I replied altogether messing up things in:
> 
> [5] [8u] RFR for backport of 8198894 (CRC32 1/4): [PPC64] More generic
> vector CRC implementation (v2)
> 
> [5]
> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-November/035729.html
> 
> 
> So in the end the final reviewed webrevs were split up into:
> 
> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-November/035766.html
> 
> 1/4 and 2/4 reviewed. Webrevs:
> 
> 1/4: http://cr.openjdk.java.net/~gromero/crc32_jdk8u/for-review/v2/8198894/
> 2/4: http://cr.openjdk.java.net/~gromero/crc32_jdk8u/for-review/v2/8216376/
> 
> and the remaining ones:
> 
> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-November/035893.html
> 
> 3/4 reviewed. Webrev:
> http://cr.openjdk.java.net/~gromero/crc32_jdk8u/for-review/v3_/8216060/
> 
> https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2019-November/035894.html
> 
> 4/4 reviewed. Webrev:
> http://cr.openjdk.java.net/~gromero/crc32_jdk8u/for-review/v3_/8217459/
> 
> 
>>> It was necessary to backport it to:
>>>
>>> - Adapt the file names to OpenJDK 8u
>>
>> Fine, and doesn't warrant review on its own.
> 
> OK.
> 
> 
>>> - 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.
> 
> OK.
> 
> 
>>> - 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.
> 
> I'm not sure what you mean here. I've addressed all concerns from
> Martin. Could
> you please check the review threads and the webrevs I've pointed out
> above to
> see if they address your concern?
> 
> 
>> 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.
> 
> Could you please try with the version I've pointed out above and if the
> mismatch
> still holds point me an example? I know it happens in general, but I fail
> to see that specific case you've mentioned.
> 
> 
>> * 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?
> 
> I missed them. When the changes were pushed to 11u (or so) the Copyright
> year
> was 2018. Currently the files on 8u have 2017. Should I update the
> Copyright
> dates to 2018 or 2020? Also, do you need a new webrev or can I fix it in
> place
> before pushing?
> 
> 
> Thanks!
> 
> Best regards,
> Gustavo
> 

Ok, I think it would be good if you just posted a new RFR with the
correct webrev for just 8198894.

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