RFR (S) 8176580: [ppc, s390] CRC32C: wrong checksum result in some cases
Zoltán Majó
zoltan.majo at oracle.com
Wed Apr 12 11:07:42 UTC 2017
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