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

[Doerr, Martin]

Thanks, we plan to push it tomorrow.


-----Original Message-----
From: Zoltán Majó [mailto:zoltan.majo at oracle.com] 
Sent: Dienstag, 14. März 2017 15:02
To: Doerr, Martin <martin.doerr at sap.com>; Schmidt, Lutz <lutz.schmidt at sap.com>; hotspot-compiler-dev at openjdk.java.net
Subject: Re: RFR (S) 8176580: [ppc, s390] CRC32C: wrong checksum result in some cases

Hi Martin,
Hi Lutz,


On 03/14/2017 02:57 PM, Doerr, Martin wrote:
>
> Hi,
>
> thanks for the explanation. I don’t want to delay getting the bug 
> fixed, so I’m ok with pushing it as it is.
>
> My comments address only cleanup which could be done separately as well.
>
> @Zoltan: As it is an obvious fix for Lutz’ recently submitted change, 
> I think you can just add me as additional reviewer and push it. If you 
> prefer that we push it, just let me know. Thanks.
>

I agree with you, this fix does not need to go through JPRT. So could 
you please go ahead an push it?

Thank you!

Best regards,


Zoltan

> Best regards,
>
> Martin
>
> *From:*Schmidt, Lutz
> *Sent:* Dienstag, 14. März 2017 14:31
> *To:* Doerr, Martin <martin.doerr at sap.com>; 
> hotspot-compiler-dev at openjdk.java.net
> *Subject:* Re: RFR (S) 8176580: [ppc, s390] CRC32C: wrong checksum 
> result in some cases
>
> Thank you, Martin, for your comments.
>
> c1_LIRGenerator*.cpp
>
> ==================
>
> Yes, I intentionally kept the code close to the SPARC implementation – 
> the only existing implementation so far.
>
> templateInterpreterGenerator_s390.cpp
>
> =================================
>
> 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,
>
> Lutz
>
> 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.
>
> 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 
> <mailto: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 
> <https://bugs.openjdk.java.net/browse/JDK-8176580>
>
> Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8176580/ 
> <http://cr.openjdk.java.net/%7Elucy/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
>