RFR (S) 8176580: [ppc, s390] CRC32C: wrong checksum result in some cases
volker.simonis at gmail.com
Thu Mar 16 10:28:29 UTC 2017
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).
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.
> Best regards,
> 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: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.
> > Andrew.
More information about the hotspot-compiler-dev