RFR (S) 8176580: [ppc, s390] CRC32C: wrong checksum result in some cases
Zoltán Majó
zoltan.majo at oracle.com
Tue Apr 11 14:35:52 UTC 2017
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 openjdk.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/~lucy/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>
> >> >
> >> >
> >> >
> >> > 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/~lucy/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>> 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.
> >> > > >
> >> > >
> >> > >
> >> >
> >> >
> >>
> >>
>
More information about the hotspot-compiler-dev
mailing list