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

Schmidt, Lutz lutz.schmidt at sap.com
Tue Mar 14 13:30:49 UTC 2017

Thank you, Martin, for your comments.

Yes, I intentionally kept the code close to the SPARC implementation – the only existing implementation so far.

The reason for the difference is simple. There does not exist a “shortcut emitter” for z_sgf as it does for z_agf. As you know, an address specification consists of three components on s390: offset(indexReg, baseReg). The shortcut emitters are pure convenience. They pass a dummy value to the real emitters.

I did not implement the missing shortcut emitter because I wanted to keep the change minimal. If “the community” weighs readable over minimal, I am happy to expand my fix.

Best regards,

On 14.03.17, 10:30, "Doerr, Martin" <martin.doerr at sap.com<mailto:martin.doerr at sap.com>> wrote:

Hi Lutz,

thanks for fixing it.

I only have minor comments.
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.

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,

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!

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

More information about the hotspot-compiler-dev mailing list