RFR (S) 8176580: [ppc, s390] CRC32C: wrong checksum result in some cases

[Doerr, Martin]

Hi Lutz,

thanks for fixing it.

I only have minor comments.
c1_LIRGenerator_ppc/s390
I think that one temp register and the 2 moves are not needed, but you can keep them if you want to keep the code closer to the SPARC implementation.

templateInterpreterGenerator_s390.cpp
I don’t really like that the following instructions access the same memory location but look differently (one of them uses an Address object, but not the other one):
       __ z_agf(data,    2*wordSize, argP);  // Add byte buffer offset.
…
       __ z_sgf(dataLen, Address(argP, 2*wordSize));  // (end_index - offset)
Readability would be better if it was consistent.

Functionally, the change looks good to me.

As the change only touches ppc/s390 files and targets jdk10, there’s no need for JPRT and could also be pushed by us.
But Zoltan already volunteered to do so. Thanks, Zoltan.

Best regards,
Martin



From: hotspot-compiler-dev [mailto:hotspot-compiler-dev-bounces at openjdk.java.net] On Behalf Of Schmidt, Lutz
Sent: Dienstag, 14. März 2017 09:16
To: hotspot-compiler-dev at openjdk.java.net
Subject: RFR (S) 8176580: [ppc, s390] CRC32C: wrong checksum result in some cases

Dear all,

may I kindly request reviews for my small bugfix? And somebody volunteering as a sponsor would be welcome as well.

Bug: https://bugs.openjdk.java.net/browse/JDK-8176580
Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8176580/

Almost two weeks ago, I had contributed CRC32C intrinsic implementations for ppc and s390. The arguments passed to the intrinsics differ slightly from those passed to the CRC32 intrinsics. Instead of passing the byte array length directly, it has to be calculated from the offset and the end index values.

This difference was not accounted for in templateInterpreterGenerator_{ppc|s390}.cpp as well as in c1_LIRGenerator_{ppc|s390}.cpp. The fix just adds the missing subtraction, and adjusts some comments. For the bug to become visible, it is necessary to calculate a CRC32C checksum from a byte array being accessed with a non-zero offset. The call to the intrinsic has to originate from the interpreter or from a c1-compiled method.

This change is “platform only”. No changes to shared code.

Thank you!
Lutz

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20170314/b36e38c9/attachment.html>