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

[Gustavo Romero]

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