RFR (S) 8176580: [ppc, s390] CRC32C: wrong checksum result in some cases
Schmidt, Lutz
lutz.schmidt at sap.com
Wed Apr 12 12:14:15 UTC 2017
Hi Zoltan,
First of all: thanks for trying to push! Second: sorry for the problems you ran into.
I do not have an immediate explanation for the failure – my dev machine is MacOS/x86_64. I will try to reproduce immediately. For the time being: is there any log output that could shed some light on the issue?
Thanks,
Lutz
On 12.04.2017, 13:10, "hotspot-compiler-dev on behalf of Zoltán Majó" <hotspot-compiler-dev-bounces at openjdk.java.net on behalf of zoltan.majo at oracle.com> wrote:
P.S.: Forgot to mention: The problem does not appear on any other x86_64
platform.
On 04/12/2017 01:07 PM, Zoltán Majó wrote:
> Hi Volker,
> Hi Lutz,
>
>
> yesterday I tried to push webrev.03 using JPRT. Unfortunately, the
> TestCRC32C.java test you've modified fails on Mac OS X on x86_64. Do
> you have an idea why that could be?
>
> Thank you! Best regards,
>
>
> Zoltan
>
> On 04/11/2017 06:03 PM, Volker Simonis wrote:
>> Thanks a lot Zoltan!
>>
>>
>> On Tue, Apr 11, 2017 at 4:35 PM, Zoltán Majó <zoltan.majo at oracle.com
>> <mailto: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>
>> <mailto: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> <mailto:lutz.schmidt at sap.com
>> <mailto:lutz.schmidt at sap.com>>>
>> Cc: Andrew Haley <aph at redhat.com <mailto:aph at redhat.com>
>> <mailto:aph at redhat.com <mailto:aph at redhat.com>>>,
>> "hotspot-compiler-dev at openjdk.java.net
>> <mailto:hotspot-compiler-dev at openjdk.java.net>
>> <mailto:hotspot-compiler-dev at openjdk.java.net
>> <mailto:hotspot-compiler-dev at openjdk.java.net>>"
>> <hotspot-compiler-dev at openjdk.java.net
>> <mailto:hotspot-compiler-dev at openjdk.java.net>
>> <mailto: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>
>> <mailto: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>
>> <mailto: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/>
>> <http://cr.openjdk.java.net/%7Elucy/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>
>> <mailto: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>
>> <mailto: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/~lucy/webrevs/8176580.02/
>> <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.02/>
>> <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.02/
>> <http://cr.openjdk.java.net/%7Elucy/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>
>> <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>
>> <mailto: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>
>> <mailto: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/~lucy/webrevs/8176580.01/
>> <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.01/>
>> <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.01/
>> <http://cr.openjdk.java.net/%7Elucy/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>
>> <mailto: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>
>> <mailto: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.
>> >> > > >
>> >> > >
>> >> > >
>> >> >
>> >> >
>> >>
>> >>
>>
>>
>>
>
More information about the hotspot-compiler-dev
mailing list