RFR (S) 8176580: [ppc, s390] CRC32C: wrong checksum result in some cases
Volker Simonis
volker.simonis at gmail.com
Tue Apr 11 16:03:02 UTC 2017
Thanks a lot Zoltan!
On Tue, Apr 11, 2017 at 4:35 PM, Zoltán Majó <zoltan.majo at oracle.com> wrote:
> Hi Volker,
>
>
> On 04/11/2017 03:34 PM, Volker Simonis wrote:
>
>>
>> Hi Zoltan,
>>
>> could you please be so kind to sponsor this reviewed change for jdk10?
>>
>
> yes, of course. I'll push it today.
>
> Best regards,
>
>
> Zoltan
>
> Initially we wanted to push it ourselves because it was s390x only but now
>> that we've also touched the tests we need a sponsor.
>>
>> Thank you and best regards,
>> Volker
>>
>> ---------- Forwarded message ----------
>> From: *Volker Simonis* <volker.simonis at gmail.com <mailto:
>> volker.simonis at gmail.com>>
>> Date: Fri, Mar 31, 2017 at 10:53 AM
>> Subject: Re: RFR (S) 8176580: [ppc, s390] CRC32C: wrong checksum result
>> in some cases
>> To: "Schmidt, Lutz" <lutz.schmidt at sap.com <mailto:lutz.schmidt at sap.com>>
>> Cc: Andrew Haley <aph at redhat.com <mailto:aph at redhat.com>>, "
>> hotspot-compiler-dev at openjdk.java.net <mailto:hotspot-compiler-dev at o
>> penjdk.java.net>" <hotspot-compiler-dev at openjdk.java.net <mailto:
>> hotspot-compiler-dev at openjdk.java.net>>
>>
>>
>> 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
>> <volker.simonis at gmail.com <mailto:volker.simonis at gmail.com>> 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 <lutz.schmidt at sap.com
>> <mailto:lutz.schmidt at sap.com>> 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/ <
>> http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.03/>
>> >>
>> >> As before, the *.cpp files have not been modified.
>> >>
>> >> Best Regards,
>> >> Lutz
>> >>
>> >>
>> >>
>> >> On 21/03/2017, 17:55, "Volker Simonis" <volker.simonis at gmail.com
>> <mailto:volker.simonis at gmail.com>> 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 <
>> lutz.schmidt at sap.com <mailto:lutz.schmidt at sap.com>> 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/~lu
>> cy/webrevs/8176580.02/ <http://cr.openjdk.java.net/%7
>> Elucy/webrevs/8176580.02/>
>> >> >
>> >> > Best regards,
>> >> > Lutz
>> >> >
>> >> >
>> >> > Dr. Lutz Schmidt | SAP JVM | PI SAP CP Core | T: +49 (6227)
>> 7-42834 <tel:%2B49%20%286227%29%207-42834>
>> >> >
>> >> >
>> >> >
>> >> > On 16.03.17, 11:28, "Volker Simonis" <volker.simonis at gmail.com
>> <mailto:volker.simonis at gmail.com>> wrote:
>> >> >
>> >> > On Wed, Mar 15, 2017 at 5:55 PM, Schmidt, Lutz <
>> lutz.schmidt at sap.com <mailto:lutz.schmidt at sap.com>> wrote:
>> >> > >
>> >> > > Hi Andrew, Volker,
>> >> > >
>> >> > > What do you think about these test enhancements?
>> >> > > Webrev: http://cr.openjdk.java.net/~lu
>> cy/webrevs/8176580.01/ <http://cr.openjdk.java.net/%7
>> Elucy/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" <
>> volker.simonis at gmail.com <mailto:volker.simonis at gmail.com>> wrote:
>> >> > >
>> >> > > On Tue, Mar 14, 2017 at 7:05 PM, Andrew Haley <
>> aph at redhat.com <mailto:aph at redhat.com>> 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.
>> >> > > >
>> >> > >
>> >> > >
>> >> >
>> >> >
>> >>
>> >>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20170411/8dc6f372/attachment-0001.html>
More information about the hotspot-compiler-dev
mailing list