RFR (S) 8176580: [ppc, s390] CRC32C: wrong checksum result in some cases
Volker Simonis
volker.simonis at gmail.com
Tue Mar 28 15:01:39 UTC 2017
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> 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" <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> 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" <volker.simonis at gmail.com> wrote:
> >
> > On Wed, Mar 15, 2017 at 5:55 PM, Schmidt, Lutz <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/
> > >
> > > 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> wrote:
> > >
> > > On Tue, Mar 14, 2017 at 7:05 PM, Andrew Haley <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