RFR (S) 8176580: [ppc, s390] CRC32C: wrong checksum result in some cases
Volker Simonis
volker.simonis at gmail.com
Fri Mar 31 08:53:38 UTC 2017
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.
>> > > >
>> > >
>> > >
>> >
>> >
>>
>>
More information about the hotspot-compiler-dev
mailing list