RFR (S) 8176580: [ppc, s390] CRC32C: wrong checksum result in some cases
Volker Simonis
volker.simonis at gmail.com
Tue Apr 11 13:34:17 UTC 2017
Hi Zoltan,
could you please be so kind to sponsor this reviewed change for jdk10?
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>
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>
Cc: Andrew Haley <aph at redhat.com>, "hotspot-compiler-dev at openjdk.java.net" <
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> 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>
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.
>> > > >
>> > >
>> > >
>> >
>> >
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20170411/ec517da4/attachment-0001.html>
More information about the hotspot-compiler-dev
mailing list