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

classic Classic list List threaded Threaded
36 messages Options
12
Reply | Threaded
Open this post in threaded view
|

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

Schmidt, Lutz

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

 

Reply | Threaded
Open this post in threaded view
|

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

Zoltán Majó
Hi Lutz,


I can sponsor the change once it's reviewed.

Best regards,


Zoltan


On 03/14/2017 09:16 AM, Schmidt, Lutz wrote:

>
> 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
>

Reply | Threaded
Open this post in threaded view
|

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

Schmidt, Lutz
Thank you, Zoltan!
I expect reviews from the experts in my close vicinity in the near future.
Regards,
Lutz


Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core (SAP JVM) | T: +49 (6227) 7-42834



On 14.03.17, 09:19, "Zoltán Majó" <[hidden email]> wrote:

    Hi Lutz,
   
   
    I can sponsor the change once it's reviewed.
   
    Best regards,
   
   
    Zoltan
   
   
    On 03/14/2017 09:16 AM, Schmidt, Lutz wrote:
    >
    > 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
    >
   
   

Reply | Threaded
Open this post in threaded view
|

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

Doerr, Martin
In reply to this post by Schmidt, Lutz

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:[hidden email]] On Behalf Of Schmidt, Lutz
Sent: Dienstag, 14. März 2017 09:16
To: [hidden email]
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

 

Reply | Threaded
Open this post in threaded view
|

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

Andrew Haley
In reply to this post by Schmidt, Lutz
On 14/03/17 08:16, Schmidt, Lutz wrote:
> 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/

Is there any addition to the test suite needed for this?

Andrew.

Reply | Threaded
Open this post in threaded view
|

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

Schmidt, Lutz
Hi Andrew,

this is a very good question indeed. My answer is two-fold:

No, for this particular bug it’s not worth to create a separate test. There already exists at least one in the Java9 JCK suite:
  api/java_util/zip/CRC32C/td.html#TestAsChecksum

Yes, one might think of running a test suite subset multiple times with different parameters. In this case, -Xint and/or –Xcomp were helpful. Forcing tests to run fully interpreted or fully compiled helps in cases where a certain function, e.g. an intrinsic, is invoked via distinct code paths.

Regards,
Lutz



On 14.03.17, 11:16, "Andrew Haley" <[hidden email]> wrote:

    On 14/03/17 08:16, Schmidt, Lutz wrote:
    > 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/
   
    Is there any addition to the test suite needed for this?
   
    Andrew.
   
   

Reply | Threaded
Open this post in threaded view
|

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

Schmidt, Lutz
In reply to this post by Doerr, Martin

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" <[hidden email]> 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:[hidden email]] On Behalf Of Schmidt, Lutz
Sent: Dienstag, 14. März 2017 09:16

To: [hidden email]
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

 

Reply | Threaded
Open this post in threaded view
|

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

Doerr, Martin

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.

 

Best regards,

Martin

 

 

From: Schmidt, Lutz
Sent: Dienstag, 14. März 2017 14:31
To: Doerr, Martin <[hidden email]>; [hidden email]
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" <[hidden email]> 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 [[hidden email]] On Behalf Of Schmidt, Lutz
Sent: Dienstag, 14. März 2017 09:16

To: [hidden email]
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

 

Reply | Threaded
Open this post in threaded view
|

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

Zoltán Majó
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 <[hidden email]>;
> [hidden email]
> *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" <[hidden email]
> <mailto:[hidden email]>> 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:[hidden email]] *On Behalf Of
> *Schmidt, Lutz
> *Sent:* Dienstag, 14. März 2017 09:16
> *To:*[hidden email]
> <mailto:[hidden email]>
> *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
>

Reply | Threaded
Open this post in threaded view
|

RE: 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:[hidden email]]
Sent: Dienstag, 14. März 2017 15:02
To: Doerr, Martin <[hidden email]>; Schmidt, Lutz <[hidden email]>; [hidden email]
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 <[hidden email]>;
> [hidden email]
> *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" <[hidden email]
> <mailto:[hidden email]>> 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:[hidden email]] *On Behalf Of
> *Schmidt, Lutz
> *Sent:* Dienstag, 14. März 2017 09:16
> *To:*[hidden email]
> <mailto:[hidden email]>
> *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
>

Reply | Threaded
Open this post in threaded view
|

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

Andrew Haley
In reply to this post by Schmidt, Lutz
On 14/03/17 13:12, Schmidt, Lutz wrote:

> Yes, one might think of running a test suite subset multiple times
> with different parameters. In this case, -Xint and/or –Xcomp were
> helpful. Forcing tests to run fully interpreted or fully compiled
> helps in cases where a certain function, e.g. an intrinsic, is
> invoked via distinct code paths.

Right, so your patch should include that change to the test suite.

Andrew.

Reply | Threaded
Open this post in threaded view
|

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

Volker Simonis
On Tue, Mar 14, 2017 at 7:05 PM, Andrew Haley <[hidden email]> wrote:

> On 14/03/17 13:12, Schmidt, Lutz wrote:
>
>> Yes, one might think of running a test suite subset multiple times
>> with different parameters. In this case, -Xint and/or –Xcomp were
>> helpful. Forcing tests to run fully interpreted or fully compiled
>> helps in cases where a certain function, e.g. an intrinsic, is
>> invoked via distinct code paths.
>
> Right, so your patch should include that change to the test suite.
>

Hi Lutz,

I agree with Andrew. We should really fix the tests such that they
check the correctness of the intrinsics.

This may be tricky if all three, the interpreter, the client and the
server compiler use the same intrinsic implementation. You could
either copy the pure Java implementation into the test so that you can
compare the results of the intrinsic operation against it or you can
switch them off in the compilers with
"-XX:DisableIntrinsic=_updateBytesCRC32C
-XX:DisableIntrinsics=_updateDirectByteBufferCRC32C" and compare the
results. Not sure which solution is more practical, but I would be
really scared if we wouldn't have these test.

Regards,
Volker

> Andrew.
>
Reply | Threaded
Open this post in threaded view
|

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

Schmidt, Lutz
Hi Andrew, Volker,

What do you think about these test enhancements?
  Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8176580.01/

Please note: the cpp files in the webrev remained unchanged.

I added some improvements (as I believe) to the TestCRC32(C).java files.

In some more detail:
The test now calculates a “reference CRC value”, based on a java implementation of the CRC32 algorithm. This reference value is used to verify all other crc values, in particular during initialization and warmup. Three additional test runs check a non-zero offset with –Xint, -Xcomp -XX:-TieredCompilation (C2 only), -Xcomp -XX:+TieredCompilation (C1 + C2).

Best regards,
Lutz


On 15.03.17, 11:50, "Volker Simonis" <[hidden email]> wrote:

    On Tue, Mar 14, 2017 at 7:05 PM, Andrew Haley <[hidden email]> wrote:
    > On 14/03/17 13:12, Schmidt, Lutz wrote:
    >
    >> Yes, one might think of running a test suite subset multiple times
    >> with different parameters. In this case, -Xint and/or –Xcomp were
    >> helpful. Forcing tests to run fully interpreted or fully compiled
    >> helps in cases where a certain function, e.g. an intrinsic, is
    >> invoked via distinct code paths.
    >
    > Right, so your patch should include that change to the test suite.
    >
   
    Hi Lutz,
   
    I agree with Andrew. We should really fix the tests such that they
    check the correctness of the intrinsics.
   
    This may be tricky if all three, the interpreter, the client and the
    server compiler use the same intrinsic implementation. You could
    either copy the pure Java implementation into the test so that you can
    compare the results of the intrinsic operation against it or you can
    switch them off in the compilers with
    "-XX:DisableIntrinsic=_updateBytesCRC32C
    -XX:DisableIntrinsics=_updateDirectByteBufferCRC32C" and compare the
    results. Not sure which solution is more practical, but I would be
    really scared if we wouldn't have these test.
   
    Regards,
    Volker
   
    > Andrew.
    >
   

Reply | Threaded
Open this post in threaded view
|

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

Volker Simonis
On Wed, Mar 15, 2017 at 5:55 PM, Schmidt, Lutz <[hidden email]> wrote:

>
> Hi Andrew, Volker,
>
> What do you think about these test enhancements?
>   Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8176580.01/
>
> Please note: the cpp files in the webrev remained unchanged.
>
> I added some improvements (as I believe) to the TestCRC32(C).java files.
>
> In some more detail:
> The test now calculates a “reference CRC value”, based on a java implementation of the CRC32 algorithm. This reference value is used to verify all other crc values, in particular during initialization and warmup. Three additional test runs check a non-zero offset with –Xint, -Xcomp -XX:-TieredCompilation (C2 only), -Xcomp -XX:+TieredCompilation (C1 + C2).
>

Hi Lutz,

thanks for updating the tests. I've had a closer look at the tests and
realized that they actually can never fail! The check() routine just
prints an error message but that will not let the test fail. So I
would suggest to throw a runtime exception in the check() routine
after the error message was printed.

I also suggest to do the check during the normal test execution (i.e.
in test_multi()) so there's no need for extra test runs.

Finally, the current test methodology in test_multi() is broken:
 - it sets the reference by calling CRC from the interpreter which
won't work if the intrinsic is also used in the interpreter.
 - it only compares the reference against the last computation of CRC
in the loop which will be the result of the C2 generated code. This
misses errors in C1.

I suggest to use your new, pure Java implementation for the
computation of the reference result and compare the reference with the
result of calling CRC in every iteration of the loop so we really
check all possibilities from interpreter trough C1 to C2.

Finally, can you please pay attention to not insert trailing
whitespace (there was some at line 88 in TestCRC32C.java). You can
easily verify this by running jcheck before creating the webrevs.

Thanks,
Volker

>
> Best regards,
> Lutz
>
>
> On 15.03.17, 11:50, "Volker Simonis" <[hidden email]> wrote:
>
>     On Tue, Mar 14, 2017 at 7:05 PM, Andrew Haley <[hidden email]> wrote:
>     > On 14/03/17 13:12, Schmidt, Lutz wrote:
>     >
>     >> Yes, one might think of running a test suite subset multiple times
>     >> with different parameters. In this case, -Xint and/or –Xcomp were
>     >> helpful. Forcing tests to run fully interpreted or fully compiled
>     >> helps in cases where a certain function, e.g. an intrinsic, is
>     >> invoked via distinct code paths.
>     >
>     > Right, so your patch should include that change to the test suite.
>     >
>
>     Hi Lutz,
>
>     I agree with Andrew. We should really fix the tests such that they
>     check the correctness of the intrinsics.
>
>     This may be tricky if all three, the interpreter, the client and the
>     server compiler use the same intrinsic implementation. You could
>     either copy the pure Java implementation into the test so that you can
>     compare the results of the intrinsic operation against it or you can
>     switch them off in the compilers with
>     "-XX:DisableIntrinsic=_updateBytesCRC32C
>     -XX:DisableIntrinsics=_updateDirectByteBufferCRC32C" and compare the
>     results. Not sure which solution is more practical, but I would be
>     really scared if we wouldn't have these test.
>
>     Regards,
>     Volker
>
>     > Andrew.
>     >
>
>
Reply | Threaded
Open this post in threaded view
|

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

Schmidt, Lutz
Hi Volker,

Thanks a lot for your valuable hints.

I have worked some time on the Java test files:
  TestCRC32.java and TestCRC32C.java are now identical as far as possible.
  They now throw an exception, should any error be detected.
  The “reference CRC value” is now used in test_multi() as well.
  The extra test runs have been removed again.
  The test methodology is fixed: each result is tested against its reference.
  The tests now detect the bug introduced with 8175368 and 8175369.
  No issue is indicated when testing with 8176580.
  I ran jcheck, and to the best of my ability and knowledge, there is no trailing whitespace.
  All *.cpp files were left untouched!

The next iteration of the webrev: http://cr.openjdk.java.net/~lucy/webrevs/8176580.02/

Best regards,
Lutz

 
Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834

 

On 16.03.17, 11:28, "Volker Simonis" <[hidden email]> wrote:

    On Wed, Mar 15, 2017 at 5:55 PM, Schmidt, Lutz <[hidden email]> wrote:
    >
    > Hi Andrew, Volker,
    >
    > What do you think about these test enhancements?
    >   Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8176580.01/
    >
    > Please note: the cpp files in the webrev remained unchanged.
    >
    > I added some improvements (as I believe) to the TestCRC32(C).java files.
    >
    > In some more detail:
    > The test now calculates a “reference CRC value”, based on a java implementation of the CRC32 algorithm. This reference value is used to verify all other crc values, in particular during initialization and warmup. Three additional test runs check a non-zero offset with –Xint, -Xcomp -XX:-TieredCompilation (C2 only), -Xcomp -XX:+TieredCompilation (C1 + C2).
    >
   
    Hi Lutz,
   
    thanks for updating the tests. I've had a closer look at the tests and
    realized that they actually can never fail! The check() routine just
    prints an error message but that will not let the test fail. So I
    would suggest to throw a runtime exception in the check() routine
    after the error message was printed.
   
    I also suggest to do the check during the normal test execution (i.e.
    in test_multi()) so there's no need for extra test runs.
   
    Finally, the current test methodology in test_multi() is broken:
     - it sets the reference by calling CRC from the interpreter which
    won't work if the intrinsic is also used in the interpreter.
     - it only compares the reference against the last computation of CRC
    in the loop which will be the result of the C2 generated code. This
    misses errors in C1.
   
    I suggest to use your new, pure Java implementation for the
    computation of the reference result and compare the reference with the
    result of calling CRC in every iteration of the loop so we really
    check all possibilities from interpreter trough C1 to C2.
   
    Finally, can you please pay attention to not insert trailing
    whitespace (there was some at line 88 in TestCRC32C.java). You can
    easily verify this by running jcheck before creating the webrevs.
   
    Thanks,
    Volker
   
    >
    > Best regards,
    > Lutz
    >
    >
    > On 15.03.17, 11:50, "Volker Simonis" <[hidden email]> wrote:
    >
    >     On Tue, Mar 14, 2017 at 7:05 PM, Andrew Haley <[hidden email]> wrote:
    >     > On 14/03/17 13:12, Schmidt, Lutz wrote:
    >     >
    >     >> Yes, one might think of running a test suite subset multiple times
    >     >> with different parameters. In this case, -Xint and/or –Xcomp were
    >     >> helpful. Forcing tests to run fully interpreted or fully compiled
    >     >> helps in cases where a certain function, e.g. an intrinsic, is
    >     >> invoked via distinct code paths.
    >     >
    >     > Right, so your patch should include that change to the test suite.
    >     >
    >
    >     Hi Lutz,
    >
    >     I agree with Andrew. We should really fix the tests such that they
    >     check the correctness of the intrinsics.
    >
    >     This may be tricky if all three, the interpreter, the client and the
    >     server compiler use the same intrinsic implementation. You could
    >     either copy the pure Java implementation into the test so that you can
    >     compare the results of the intrinsic operation against it or you can
    >     switch them off in the compilers with
    >     "-XX:DisableIntrinsic=_updateBytesCRC32C
    >     -XX:DisableIntrinsics=_updateDirectByteBufferCRC32C" and compare the
    >     results. Not sure which solution is more practical, but I would be
    >     really scared if we wouldn't have these test.
    >
    >     Regards,
    >     Volker
    >
    >     > Andrew.
    >     >
    >
    >
   

Reply | Threaded
Open this post in threaded view
|

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

Volker Simonis
Hi Lutz,

thanks a lot for updating the tests. I think they look much better now.

There's just one more cleanup I'd like to propose. Can you please move
the throw right into the check() function. Just make check() return
void and throw from it if there's a mismatch between the computed and
the expected result. I leave it up to you if you want to pass an extra
error string to check() which will be printed in the case of an error.
I personally don't think that's necessary as it will be evident from
the stack trace which computation failed.

Also the try/catch and rethrow in test_multi() isn't necessary. The
test can be simply terminated by the initial exception.

Thank you and best regards,
Volker


On Fri, Mar 17, 2017 at 10:03 PM, Schmidt, Lutz <[hidden email]> wrote:

> Hi Volker,
>
> Thanks a lot for your valuable hints.
>
> I have worked some time on the Java test files:
>   TestCRC32.java and TestCRC32C.java are now identical as far as possible.
>   They now throw an exception, should any error be detected.
>   The “reference CRC value” is now used in test_multi() as well.
>   The extra test runs have been removed again.
>   The test methodology is fixed: each result is tested against its reference.
>   The tests now detect the bug introduced with 8175368 and 8175369.
>   No issue is indicated when testing with 8176580.
>   I ran jcheck, and to the best of my ability and knowledge, there is no trailing whitespace.
>   All *.cpp files were left untouched!
>
> The next iteration of the webrev: http://cr.openjdk.java.net/~lucy/webrevs/8176580.02/
>
> Best regards,
> Lutz
>
>
> Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834
>
>
>
> On 16.03.17, 11:28, "Volker Simonis" <[hidden email]> wrote:
>
>     On Wed, Mar 15, 2017 at 5:55 PM, Schmidt, Lutz <[hidden email]> wrote:
>     >
>     > Hi Andrew, Volker,
>     >
>     > What do you think about these test enhancements?
>     >   Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8176580.01/
>     >
>     > Please note: the cpp files in the webrev remained unchanged.
>     >
>     > I added some improvements (as I believe) to the TestCRC32(C).java files.
>     >
>     > In some more detail:
>     > The test now calculates a “reference CRC value”, based on a java implementation of the CRC32 algorithm. This reference value is used to verify all other crc values, in particular during initialization and warmup. Three additional test runs check a non-zero offset with –Xint, -Xcomp -XX:-TieredCompilation (C2 only), -Xcomp -XX:+TieredCompilation (C1 + C2).
>     >
>
>     Hi Lutz,
>
>     thanks for updating the tests. I've had a closer look at the tests and
>     realized that they actually can never fail! The check() routine just
>     prints an error message but that will not let the test fail. So I
>     would suggest to throw a runtime exception in the check() routine
>     after the error message was printed.
>
>     I also suggest to do the check during the normal test execution (i.e.
>     in test_multi()) so there's no need for extra test runs.
>
>     Finally, the current test methodology in test_multi() is broken:
>      - it sets the reference by calling CRC from the interpreter which
>     won't work if the intrinsic is also used in the interpreter.
>      - it only compares the reference against the last computation of CRC
>     in the loop which will be the result of the C2 generated code. This
>     misses errors in C1.
>
>     I suggest to use your new, pure Java implementation for the
>     computation of the reference result and compare the reference with the
>     result of calling CRC in every iteration of the loop so we really
>     check all possibilities from interpreter trough C1 to C2.
>
>     Finally, can you please pay attention to not insert trailing
>     whitespace (there was some at line 88 in TestCRC32C.java). You can
>     easily verify this by running jcheck before creating the webrevs.
>
>     Thanks,
>     Volker
>
>     >
>     > Best regards,
>     > Lutz
>     >
>     >
>     > On 15.03.17, 11:50, "Volker Simonis" <[hidden email]> wrote:
>     >
>     >     On Tue, Mar 14, 2017 at 7:05 PM, Andrew Haley <[hidden email]> wrote:
>     >     > On 14/03/17 13:12, Schmidt, Lutz wrote:
>     >     >
>     >     >> Yes, one might think of running a test suite subset multiple times
>     >     >> with different parameters. In this case, -Xint and/or –Xcomp were
>     >     >> helpful. Forcing tests to run fully interpreted or fully compiled
>     >     >> helps in cases where a certain function, e.g. an intrinsic, is
>     >     >> invoked via distinct code paths.
>     >     >
>     >     > Right, so your patch should include that change to the test suite.
>     >     >
>     >
>     >     Hi Lutz,
>     >
>     >     I agree with Andrew. We should really fix the tests such that they
>     >     check the correctness of the intrinsics.
>     >
>     >     This may be tricky if all three, the interpreter, the client and the
>     >     server compiler use the same intrinsic implementation. You could
>     >     either copy the pure Java implementation into the test so that you can
>     >     compare the results of the intrinsic operation against it or you can
>     >     switch them off in the compilers with
>     >     "-XX:DisableIntrinsic=_updateBytesCRC32C
>     >     -XX:DisableIntrinsics=_updateDirectByteBufferCRC32C" and compare the
>     >     results. Not sure which solution is more practical, but I would be
>     >     really scared if we wouldn't have these test.
>     >
>     >     Regards,
>     >     Volker
>     >
>     >     > Andrew.
>     >     >
>     >
>     >
>
>
Reply | Threaded
Open this post in threaded view
|

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

Schmidt, Lutz
Hi Volker,

Sorry for letting you wait. Here is the final(?) webrev, containing all your requests for cleanup and improvements:
http://cr.openjdk.java.net/~lucy/webrevs/8176580.03/

As before, the *.cpp files have not been modified.

Best Regards,
Lutz

 

On 21/03/2017, 17:55, "Volker Simonis" <[hidden email]> wrote:

    Hi Lutz,
   
    thanks a lot for updating the tests. I think they look much better now.
   
    There's just one more cleanup I'd like to propose. Can you please move
    the throw right into the check() function. Just make check() return
    void and throw from it if there's a mismatch between the computed and
    the expected result. I leave it up to you if you want to pass an extra
    error string to check() which will be printed in the case of an error.
    I personally don't think that's necessary as it will be evident from
    the stack trace which computation failed.
   
    Also the try/catch and rethrow in test_multi() isn't necessary. The
    test can be simply terminated by the initial exception.
   
    Thank you and best regards,
    Volker
   
   
    On Fri, Mar 17, 2017 at 10:03 PM, Schmidt, Lutz <[hidden email]> wrote:
    > Hi Volker,
    >
    > Thanks a lot for your valuable hints.
    >
    > I have worked some time on the Java test files:
    >   TestCRC32.java and TestCRC32C.java are now identical as far as possible.
    >   They now throw an exception, should any error be detected.
    >   The “reference CRC value” is now used in test_multi() as well.
    >   The extra test runs have been removed again.
    >   The test methodology is fixed: each result is tested against its reference.
    >   The tests now detect the bug introduced with 8175368 and 8175369.
    >   No issue is indicated when testing with 8176580.
    >   I ran jcheck, and to the best of my ability and knowledge, there is no trailing whitespace.
    >   All *.cpp files were left untouched!
    >
    > The next iteration of the webrev: http://cr.openjdk.java.net/~lucy/webrevs/8176580.02/
    >
    > Best regards,
    > Lutz
    >
    >
    > Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834
    >
    >
    >
    > On 16.03.17, 11:28, "Volker Simonis" <[hidden email]> wrote:
    >
    >     On Wed, Mar 15, 2017 at 5:55 PM, Schmidt, Lutz <[hidden email]> wrote:
    >     >
    >     > Hi Andrew, Volker,
    >     >
    >     > What do you think about these test enhancements?
    >     >   Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8176580.01/
    >     >
    >     > Please note: the cpp files in the webrev remained unchanged.
    >     >
    >     > I added some improvements (as I believe) to the TestCRC32(C).java files.
    >     >
    >     > In some more detail:
    >     > The test now calculates a “reference CRC value”, based on a java implementation of the CRC32 algorithm. This reference value is used to verify all other crc values, in particular during initialization and warmup. Three additional test runs check a non-zero offset with –Xint, -Xcomp -XX:-TieredCompilation (C2 only), -Xcomp -XX:+TieredCompilation (C1 + C2).
    >     >
    >
    >     Hi Lutz,
    >
    >     thanks for updating the tests. I've had a closer look at the tests and
    >     realized that they actually can never fail! The check() routine just
    >     prints an error message but that will not let the test fail. So I
    >     would suggest to throw a runtime exception in the check() routine
    >     after the error message was printed.
    >
    >     I also suggest to do the check during the normal test execution (i.e.
    >     in test_multi()) so there's no need for extra test runs.
    >
    >     Finally, the current test methodology in test_multi() is broken:
    >      - it sets the reference by calling CRC from the interpreter which
    >     won't work if the intrinsic is also used in the interpreter.
    >      - it only compares the reference against the last computation of CRC
    >     in the loop which will be the result of the C2 generated code. This
    >     misses errors in C1.
    >
    >     I suggest to use your new, pure Java implementation for the
    >     computation of the reference result and compare the reference with the
    >     result of calling CRC in every iteration of the loop so we really
    >     check all possibilities from interpreter trough C1 to C2.
    >
    >     Finally, can you please pay attention to not insert trailing
    >     whitespace (there was some at line 88 in TestCRC32C.java). You can
    >     easily verify this by running jcheck before creating the webrevs.
    >
    >     Thanks,
    >     Volker
    >
    >     >
    >     > Best regards,
    >     > Lutz
    >     >
    >     >
    >     > On 15.03.17, 11:50, "Volker Simonis" <[hidden email]> wrote:
    >     >
    >     >     On Tue, Mar 14, 2017 at 7:05 PM, Andrew Haley <[hidden email]> wrote:
    >     >     > On 14/03/17 13:12, Schmidt, Lutz wrote:
    >     >     >
    >     >     >> Yes, one might think of running a test suite subset multiple times
    >     >     >> with different parameters. In this case, -Xint and/or –Xcomp were
    >     >     >> helpful. Forcing tests to run fully interpreted or fully compiled
    >     >     >> helps in cases where a certain function, e.g. an intrinsic, is
    >     >     >> invoked via distinct code paths.
    >     >     >
    >     >     > Right, so your patch should include that change to the test suite.
    >     >     >
    >     >
    >     >     Hi Lutz,
    >     >
    >     >     I agree with Andrew. We should really fix the tests such that they
    >     >     check the correctness of the intrinsics.
    >     >
    >     >     This may be tricky if all three, the interpreter, the client and the
    >     >     server compiler use the same intrinsic implementation. You could
    >     >     either copy the pure Java implementation into the test so that you can
    >     >     compare the results of the intrinsic operation against it or you can
    >     >     switch them off in the compilers with
    >     >     "-XX:DisableIntrinsic=_updateBytesCRC32C
    >     >     -XX:DisableIntrinsics=_updateDirectByteBufferCRC32C" and compare the
    >     >     results. Not sure which solution is more practical, but I would be
    >     >     really scared if we wouldn't have these test.
    >     >
    >     >     Regards,
    >     >     Volker
    >     >
    >     >     > Andrew.
    >     >     >
    >     >
    >     >
    >
    >
   

Reply | Threaded
Open this post in threaded view
|

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

Volker Simonis
Hi Lutz,

thanks a lot for fixing the test!
Your change looks good now.

Because this touches shared (i.e. test) files, we still need a sponsor
so can somebody please sponsor this change?

Thank you and best regards,
Volker



On Fri, Mar 24, 2017 at 4:54 PM, Schmidt, Lutz <[hidden email]> wrote:

> Hi Volker,
>
> Sorry for letting you wait. Here is the final(?) webrev, containing all your requests for cleanup and improvements:
> http://cr.openjdk.java.net/~lucy/webrevs/8176580.03/
>
> As before, the *.cpp files have not been modified.
>
> Best Regards,
> Lutz
>
>
>
> On 21/03/2017, 17:55, "Volker Simonis" <[hidden email]> wrote:
>
>     Hi Lutz,
>
>     thanks a lot for updating the tests. I think they look much better now.
>
>     There's just one more cleanup I'd like to propose. Can you please move
>     the throw right into the check() function. Just make check() return
>     void and throw from it if there's a mismatch between the computed and
>     the expected result. I leave it up to you if you want to pass an extra
>     error string to check() which will be printed in the case of an error.
>     I personally don't think that's necessary as it will be evident from
>     the stack trace which computation failed.
>
>     Also the try/catch and rethrow in test_multi() isn't necessary. The
>     test can be simply terminated by the initial exception.
>
>     Thank you and best regards,
>     Volker
>
>
>     On Fri, Mar 17, 2017 at 10:03 PM, Schmidt, Lutz <[hidden email]> wrote:
>     > Hi Volker,
>     >
>     > Thanks a lot for your valuable hints.
>     >
>     > I have worked some time on the Java test files:
>     >   TestCRC32.java and TestCRC32C.java are now identical as far as possible.
>     >   They now throw an exception, should any error be detected.
>     >   The “reference CRC value” is now used in test_multi() as well.
>     >   The extra test runs have been removed again.
>     >   The test methodology is fixed: each result is tested against its reference.
>     >   The tests now detect the bug introduced with 8175368 and 8175369.
>     >   No issue is indicated when testing with 8176580.
>     >   I ran jcheck, and to the best of my ability and knowledge, there is no trailing whitespace.
>     >   All *.cpp files were left untouched!
>     >
>     > The next iteration of the webrev: http://cr.openjdk.java.net/~lucy/webrevs/8176580.02/
>     >
>     > Best regards,
>     > Lutz
>     >
>     >
>     > Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834
>     >
>     >
>     >
>     > On 16.03.17, 11:28, "Volker Simonis" <[hidden email]> wrote:
>     >
>     >     On Wed, Mar 15, 2017 at 5:55 PM, Schmidt, Lutz <[hidden email]> wrote:
>     >     >
>     >     > Hi Andrew, Volker,
>     >     >
>     >     > What do you think about these test enhancements?
>     >     >   Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8176580.01/
>     >     >
>     >     > Please note: the cpp files in the webrev remained unchanged.
>     >     >
>     >     > I added some improvements (as I believe) to the TestCRC32(C).java files.
>     >     >
>     >     > In some more detail:
>     >     > The test now calculates a “reference CRC value”, based on a java implementation of the CRC32 algorithm. This reference value is used to verify all other crc values, in particular during initialization and warmup. Three additional test runs check a non-zero offset with –Xint, -Xcomp -XX:-TieredCompilation (C2 only), -Xcomp -XX:+TieredCompilation (C1 + C2).
>     >     >
>     >
>     >     Hi Lutz,
>     >
>     >     thanks for updating the tests. I've had a closer look at the tests and
>     >     realized that they actually can never fail! The check() routine just
>     >     prints an error message but that will not let the test fail. So I
>     >     would suggest to throw a runtime exception in the check() routine
>     >     after the error message was printed.
>     >
>     >     I also suggest to do the check during the normal test execution (i.e.
>     >     in test_multi()) so there's no need for extra test runs.
>     >
>     >     Finally, the current test methodology in test_multi() is broken:
>     >      - it sets the reference by calling CRC from the interpreter which
>     >     won't work if the intrinsic is also used in the interpreter.
>     >      - it only compares the reference against the last computation of CRC
>     >     in the loop which will be the result of the C2 generated code. This
>     >     misses errors in C1.
>     >
>     >     I suggest to use your new, pure Java implementation for the
>     >     computation of the reference result and compare the reference with the
>     >     result of calling CRC in every iteration of the loop so we really
>     >     check all possibilities from interpreter trough C1 to C2.
>     >
>     >     Finally, can you please pay attention to not insert trailing
>     >     whitespace (there was some at line 88 in TestCRC32C.java). You can
>     >     easily verify this by running jcheck before creating the webrevs.
>     >
>     >     Thanks,
>     >     Volker
>     >
>     >     >
>     >     > Best regards,
>     >     > Lutz
>     >     >
>     >     >
>     >     > On 15.03.17, 11:50, "Volker Simonis" <[hidden email]> wrote:
>     >     >
>     >     >     On Tue, Mar 14, 2017 at 7:05 PM, Andrew Haley <[hidden email]> wrote:
>     >     >     > On 14/03/17 13:12, Schmidt, Lutz wrote:
>     >     >     >
>     >     >     >> Yes, one might think of running a test suite subset multiple times
>     >     >     >> with different parameters. In this case, -Xint and/or –Xcomp were
>     >     >     >> helpful. Forcing tests to run fully interpreted or fully compiled
>     >     >     >> helps in cases where a certain function, e.g. an intrinsic, is
>     >     >     >> invoked via distinct code paths.
>     >     >     >
>     >     >     > Right, so your patch should include that change to the test suite.
>     >     >     >
>     >     >
>     >     >     Hi Lutz,
>     >     >
>     >     >     I agree with Andrew. We should really fix the tests such that they
>     >     >     check the correctness of the intrinsics.
>     >     >
>     >     >     This may be tricky if all three, the interpreter, the client and the
>     >     >     server compiler use the same intrinsic implementation. You could
>     >     >     either copy the pure Java implementation into the test so that you can
>     >     >     compare the results of the intrinsic operation against it or you can
>     >     >     switch them off in the compilers with
>     >     >     "-XX:DisableIntrinsic=_updateBytesCRC32C
>     >     >     -XX:DisableIntrinsics=_updateDirectByteBufferCRC32C" and compare the
>     >     >     results. Not sure which solution is more practical, but I would be
>     >     >     really scared if we wouldn't have these test.
>     >     >
>     >     >     Regards,
>     >     >     Volker
>     >     >
>     >     >     > Andrew.
>     >     >     >
>     >     >
>     >     >
>     >
>     >
>
>
Reply | Threaded
Open this post in threaded view
|

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

Volker Simonis
Ping...

Can somebody please push this change?

It's ppc64/s390x only but as a courtesy to the community it also fixes
the CRC JTreg tests so unfortunately I still can't push it myself :)

Thank you and best regards,
Volker


On Tue, Mar 28, 2017 at 5:01 PM, Volker Simonis
<[hidden email]> wrote:

> Hi Lutz,
>
> thanks a lot for fixing the test!
> Your change looks good now.
>
> Because this touches shared (i.e. test) files, we still need a sponsor
> so can somebody please sponsor this change?
>
> Thank you and best regards,
> Volker
>
>
>
> On Fri, Mar 24, 2017 at 4:54 PM, Schmidt, Lutz <[hidden email]> wrote:
>> Hi Volker,
>>
>> Sorry for letting you wait. Here is the final(?) webrev, containing all your requests for cleanup and improvements:
>> http://cr.openjdk.java.net/~lucy/webrevs/8176580.03/
>>
>> As before, the *.cpp files have not been modified.
>>
>> Best Regards,
>> Lutz
>>
>>
>>
>> On 21/03/2017, 17:55, "Volker Simonis" <[hidden email]> wrote:
>>
>>     Hi Lutz,
>>
>>     thanks a lot for updating the tests. I think they look much better now.
>>
>>     There's just one more cleanup I'd like to propose. Can you please move
>>     the throw right into the check() function. Just make check() return
>>     void and throw from it if there's a mismatch between the computed and
>>     the expected result. I leave it up to you if you want to pass an extra
>>     error string to check() which will be printed in the case of an error.
>>     I personally don't think that's necessary as it will be evident from
>>     the stack trace which computation failed.
>>
>>     Also the try/catch and rethrow in test_multi() isn't necessary. The
>>     test can be simply terminated by the initial exception.
>>
>>     Thank you and best regards,
>>     Volker
>>
>>
>>     On Fri, Mar 17, 2017 at 10:03 PM, Schmidt, Lutz <[hidden email]> wrote:
>>     > Hi Volker,
>>     >
>>     > Thanks a lot for your valuable hints.
>>     >
>>     > I have worked some time on the Java test files:
>>     >   TestCRC32.java and TestCRC32C.java are now identical as far as possible.
>>     >   They now throw an exception, should any error be detected.
>>     >   The “reference CRC value” is now used in test_multi() as well.
>>     >   The extra test runs have been removed again.
>>     >   The test methodology is fixed: each result is tested against its reference.
>>     >   The tests now detect the bug introduced with 8175368 and 8175369.
>>     >   No issue is indicated when testing with 8176580.
>>     >   I ran jcheck, and to the best of my ability and knowledge, there is no trailing whitespace.
>>     >   All *.cpp files were left untouched!
>>     >
>>     > The next iteration of the webrev: http://cr.openjdk.java.net/~lucy/webrevs/8176580.02/
>>     >
>>     > Best regards,
>>     > Lutz
>>     >
>>     >
>>     > Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834
>>     >
>>     >
>>     >
>>     > On 16.03.17, 11:28, "Volker Simonis" <[hidden email]> wrote:
>>     >
>>     >     On Wed, Mar 15, 2017 at 5:55 PM, Schmidt, Lutz <[hidden email]> wrote:
>>     >     >
>>     >     > Hi Andrew, Volker,
>>     >     >
>>     >     > What do you think about these test enhancements?
>>     >     >   Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8176580.01/
>>     >     >
>>     >     > Please note: the cpp files in the webrev remained unchanged.
>>     >     >
>>     >     > I added some improvements (as I believe) to the TestCRC32(C).java files.
>>     >     >
>>     >     > In some more detail:
>>     >     > The test now calculates a “reference CRC value”, based on a java implementation of the CRC32 algorithm. This reference value is used to verify all other crc values, in particular during initialization and warmup. Three additional test runs check a non-zero offset with –Xint, -Xcomp -XX:-TieredCompilation (C2 only), -Xcomp -XX:+TieredCompilation (C1 + C2).
>>     >     >
>>     >
>>     >     Hi Lutz,
>>     >
>>     >     thanks for updating the tests. I've had a closer look at the tests and
>>     >     realized that they actually can never fail! The check() routine just
>>     >     prints an error message but that will not let the test fail. So I
>>     >     would suggest to throw a runtime exception in the check() routine
>>     >     after the error message was printed.
>>     >
>>     >     I also suggest to do the check during the normal test execution (i.e.
>>     >     in test_multi()) so there's no need for extra test runs.
>>     >
>>     >     Finally, the current test methodology in test_multi() is broken:
>>     >      - it sets the reference by calling CRC from the interpreter which
>>     >     won't work if the intrinsic is also used in the interpreter.
>>     >      - it only compares the reference against the last computation of CRC
>>     >     in the loop which will be the result of the C2 generated code. This
>>     >     misses errors in C1.
>>     >
>>     >     I suggest to use your new, pure Java implementation for the
>>     >     computation of the reference result and compare the reference with the
>>     >     result of calling CRC in every iteration of the loop so we really
>>     >     check all possibilities from interpreter trough C1 to C2.
>>     >
>>     >     Finally, can you please pay attention to not insert trailing
>>     >     whitespace (there was some at line 88 in TestCRC32C.java). You can
>>     >     easily verify this by running jcheck before creating the webrevs.
>>     >
>>     >     Thanks,
>>     >     Volker
>>     >
>>     >     >
>>     >     > Best regards,
>>     >     > Lutz
>>     >     >
>>     >     >
>>     >     > On 15.03.17, 11:50, "Volker Simonis" <[hidden email]> wrote:
>>     >     >
>>     >     >     On Tue, Mar 14, 2017 at 7:05 PM, Andrew Haley <[hidden email]> wrote:
>>     >     >     > On 14/03/17 13:12, Schmidt, Lutz wrote:
>>     >     >     >
>>     >     >     >> Yes, one might think of running a test suite subset multiple times
>>     >     >     >> with different parameters. In this case, -Xint and/or –Xcomp were
>>     >     >     >> helpful. Forcing tests to run fully interpreted or fully compiled
>>     >     >     >> helps in cases where a certain function, e.g. an intrinsic, is
>>     >     >     >> invoked via distinct code paths.
>>     >     >     >
>>     >     >     > Right, so your patch should include that change to the test suite.
>>     >     >     >
>>     >     >
>>     >     >     Hi Lutz,
>>     >     >
>>     >     >     I agree with Andrew. We should really fix the tests such that they
>>     >     >     check the correctness of the intrinsics.
>>     >     >
>>     >     >     This may be tricky if all three, the interpreter, the client and the
>>     >     >     server compiler use the same intrinsic implementation. You could
>>     >     >     either copy the pure Java implementation into the test so that you can
>>     >     >     compare the results of the intrinsic operation against it or you can
>>     >     >     switch them off in the compilers with
>>     >     >     "-XX:DisableIntrinsic=_updateBytesCRC32C
>>     >     >     -XX:DisableIntrinsics=_updateDirectByteBufferCRC32C" and compare the
>>     >     >     results. Not sure which solution is more practical, but I would be
>>     >     >     really scared if we wouldn't have these test.
>>     >     >
>>     >     >     Regards,
>>     >     >     Volker
>>     >     >
>>     >     >     > Andrew.
>>     >     >     >
>>     >     >
>>     >     >
>>     >
>>     >
>>
>>
Reply | Threaded
Open this post in threaded view
|

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

Schmidt, Lutz
Thanks, Volker,

for guiding me to the right path and for taking care of the sponsoring.

Regards, Lutz


 

On 31.03.2017, 10:53, "Volker Simonis" <[hidden email]> wrote:

    Ping...
   
    Can somebody please push this change?
   
    It's ppc64/s390x only but as a courtesy to the community it also fixes
    the CRC JTreg tests so unfortunately I still can't push it myself :)
   
    Thank you and best regards,
    Volker
   
   
    On Tue, Mar 28, 2017 at 5:01 PM, Volker Simonis
    <[hidden email]> wrote:
    > Hi Lutz,
    >
    > thanks a lot for fixing the test!
    > Your change looks good now.
    >
    > Because this touches shared (i.e. test) files, we still need a sponsor
    > so can somebody please sponsor this change?
    >
    > Thank you and best regards,
    > Volker
    >
    >
    >
    > On Fri, Mar 24, 2017 at 4:54 PM, Schmidt, Lutz <[hidden email]> wrote:
    >> Hi Volker,
    >>
    >> Sorry for letting you wait. Here is the final(?) webrev, containing all your requests for cleanup and improvements:
    >> http://cr.openjdk.java.net/~lucy/webrevs/8176580.03/
    >>
    >> As before, the *.cpp files have not been modified.
    >>
    >> Best Regards,
    >> Lutz
    >>
    >>
    >>
    >> On 21/03/2017, 17:55, "Volker Simonis" <[hidden email]> wrote:
    >>
    >>     Hi Lutz,
    >>
    >>     thanks a lot for updating the tests. I think they look much better now.
    >>
    >>     There's just one more cleanup I'd like to propose. Can you please move
    >>     the throw right into the check() function. Just make check() return
    >>     void and throw from it if there's a mismatch between the computed and
    >>     the expected result. I leave it up to you if you want to pass an extra
    >>     error string to check() which will be printed in the case of an error.
    >>     I personally don't think that's necessary as it will be evident from
    >>     the stack trace which computation failed.
    >>
    >>     Also the try/catch and rethrow in test_multi() isn't necessary. The
    >>     test can be simply terminated by the initial exception.
    >>
    >>     Thank you and best regards,
    >>     Volker
    >>
    >>
    >>     On Fri, Mar 17, 2017 at 10:03 PM, Schmidt, Lutz <[hidden email]> wrote:
    >>     > Hi Volker,
    >>     >
    >>     > Thanks a lot for your valuable hints.
    >>     >
    >>     > I have worked some time on the Java test files:
    >>     >   TestCRC32.java and TestCRC32C.java are now identical as far as possible.
    >>     >   They now throw an exception, should any error be detected.
    >>     >   The “reference CRC value” is now used in test_multi() as well.
    >>     >   The extra test runs have been removed again.
    >>     >   The test methodology is fixed: each result is tested against its reference.
    >>     >   The tests now detect the bug introduced with 8175368 and 8175369.
    >>     >   No issue is indicated when testing with 8176580.
    >>     >   I ran jcheck, and to the best of my ability and knowledge, there is no trailing whitespace.
    >>     >   All *.cpp files were left untouched!
    >>     >
    >>     > The next iteration of the webrev: http://cr.openjdk.java.net/~lucy/webrevs/8176580.02/
    >>     >
    >>     > Best regards,
    >>     > Lutz
    >>     >
    >>     >
    >>     > Dr. Lutz Schmidt | SAP JVM | PI  SAP CP Core | T: +49 (6227) 7-42834
    >>     >
    >>     >
    >>     >
    >>     > On 16.03.17, 11:28, "Volker Simonis" <[hidden email]> wrote:
    >>     >
    >>     >     On Wed, Mar 15, 2017 at 5:55 PM, Schmidt, Lutz <[hidden email]> wrote:
    >>     >     >
    >>     >     > Hi Andrew, Volker,
    >>     >     >
    >>     >     > What do you think about these test enhancements?
    >>     >     >   Webrev: http://cr.openjdk.java.net/~lucy/webrevs/8176580.01/
    >>     >     >
    >>     >     > Please note: the cpp files in the webrev remained unchanged.
    >>     >     >
    >>     >     > I added some improvements (as I believe) to the TestCRC32(C).java files.
    >>     >     >
    >>     >     > In some more detail:
    >>     >     > The test now calculates a “reference CRC value”, based on a java implementation of the CRC32 algorithm. This reference value is used to verify all other crc values, in particular during initialization and warmup. Three additional test runs check a non-zero offset with –Xint, -Xcomp -XX:-TieredCompilation (C2 only), -Xcomp -XX:+TieredCompilation (C1 + C2).
    >>     >     >
    >>     >
    >>     >     Hi Lutz,
    >>     >
    >>     >     thanks for updating the tests. I've had a closer look at the tests and
    >>     >     realized that they actually can never fail! The check() routine just
    >>     >     prints an error message but that will not let the test fail. So I
    >>     >     would suggest to throw a runtime exception in the check() routine
    >>     >     after the error message was printed.
    >>     >
    >>     >     I also suggest to do the check during the normal test execution (i.e.
    >>     >     in test_multi()) so there's no need for extra test runs.
    >>     >
    >>     >     Finally, the current test methodology in test_multi() is broken:
    >>     >      - it sets the reference by calling CRC from the interpreter which
    >>     >     won't work if the intrinsic is also used in the interpreter.
    >>     >      - it only compares the reference against the last computation of CRC
    >>     >     in the loop which will be the result of the C2 generated code. This
    >>     >     misses errors in C1.
    >>     >
    >>     >     I suggest to use your new, pure Java implementation for the
    >>     >     computation of the reference result and compare the reference with the
    >>     >     result of calling CRC in every iteration of the loop so we really
    >>     >     check all possibilities from interpreter trough C1 to C2.
    >>     >
    >>     >     Finally, can you please pay attention to not insert trailing
    >>     >     whitespace (there was some at line 88 in TestCRC32C.java). You can
    >>     >     easily verify this by running jcheck before creating the webrevs.
    >>     >
    >>     >     Thanks,
    >>     >     Volker
    >>     >
    >>     >     >
    >>     >     > Best regards,
    >>     >     > Lutz
    >>     >     >
    >>     >     >
    >>     >     > On 15.03.17, 11:50, "Volker Simonis" <[hidden email]> wrote:
    >>     >     >
    >>     >     >     On Tue, Mar 14, 2017 at 7:05 PM, Andrew Haley <[hidden email]> wrote:
    >>     >     >     > On 14/03/17 13:12, Schmidt, Lutz wrote:
    >>     >     >     >
    >>     >     >     >> Yes, one might think of running a test suite subset multiple times
    >>     >     >     >> with different parameters. In this case, -Xint and/or –Xcomp were
    >>     >     >     >> helpful. Forcing tests to run fully interpreted or fully compiled
    >>     >     >     >> helps in cases where a certain function, e.g. an intrinsic, is
    >>     >     >     >> invoked via distinct code paths.
    >>     >     >     >
    >>     >     >     > Right, so your patch should include that change to the test suite.
    >>     >     >     >
    >>     >     >
    >>     >     >     Hi Lutz,
    >>     >     >
    >>     >     >     I agree with Andrew. We should really fix the tests such that they
    >>     >     >     check the correctness of the intrinsics.
    >>     >     >
    >>     >     >     This may be tricky if all three, the interpreter, the client and the
    >>     >     >     server compiler use the same intrinsic implementation. You could
    >>     >     >     either copy the pure Java implementation into the test so that you can
    >>     >     >     compare the results of the intrinsic operation against it or you can
    >>     >     >     switch them off in the compilers with
    >>     >     >     "-XX:DisableIntrinsic=_updateBytesCRC32C
    >>     >     >     -XX:DisableIntrinsics=_updateDirectByteBufferCRC32C" and compare the
    >>     >     >     results. Not sure which solution is more practical, but I would be
    >>     >     >     really scared if we wouldn't have these test.
    >>     >     >
    >>     >     >     Regards,
    >>     >     >     Volker
    >>     >     >
    >>     >     >     > Andrew.
    >>     >     >     >
    >>     >     >
    >>     >     >
    >>     >
    >>     >
    >>
    >>
   

12