RFR (S) 8176580: [ppc, s390] CRC32C: wrong checksum result in some cases

Zoltán Majó zoltan.majo at oracle.com
Thu Apr 13 13:39:19 UTC 2017


Hi Volker,
Hi Lutz,


On 04/13/2017 03:25 PM, Volker Simonis wrote:
> Hi Lutz, Zoltan,
>
> @Lutz, thanks for analyzing the problem! I actually didn't expected 
> that the efforts we put into fixing the tests will pay up that fast :)
>
> @Zoltan: I propose we split the test from the actual fix. I can push 
> the ppc64/s390x parts myself.

That is fine with me.

Just to clarify: Will you include the other test Lutz has modified 
(test/compiler/intrinsics/zip/TestCRC32.java) into your JDK 10 
changeset? I can live with both options (i.e., (1) both tests pushed 
into JDK 9 with 8178720 and (2) *CRC32C -> JDK 9, CRC32 -> JDK 10).

>
> I opened the new prio 2 bug "8178720: CRC32C fails on old x86 hardware 
> without CLMUL support" for jdk9

Thank you.

> and linked the reworked tests from Lutz' last webrev to it. I think 
> this is definitely something we should fix before jdk9 will be 
> released and the nfix should contain the new, fixed version of the 
> tests. Zoltan, can you please take care of this?

Yes, I can. Just let me know which option from above you prefer.

For the record: I'll add Lutz as a contributor when pushing the tests 
(if that's fine with you, Lutz).

Best regards,


Zoltan

>
> Thank you and best regards,
> Volker
>
> [1] https://bugs.openjdk.java.net/browse/JDK-8178720
>
>
> On Thu, Apr 13, 2017 at 12:10 PM, Schmidt, Lutz <lutz.schmidt at sap.com 
> <mailto:lutz.schmidt at sap.com>> wrote:
>
>     Hi Zoltan,
>
>     I am now able to reproduce the problem. Calculated crc and
>     reference crc are exactly the same as in the log output you pasted
>     below. And the problem shows up on linux-x86_64 as well.
>
>     Here is what’s happening:
>     The CRC32C implementation generates code depending on the CLMUL
>     cpu feature being available. On most machines @Oracle and @SAP,
>     this feature seems to be available – except for one of your MacOS
>     test machines.
>
>     To gain control over that feature, I have modified just one line
>     in src/cpu/x86/vm/vm_version_x86_64.hpp like this:
>          static bool supports_clmul()    { return ((_features &
>     CPU_CLMUL) != 0) && UseNewCode; }
>
>     The boolean UseNewCode is a command line parameter we use @SAP to
>     make experimental code switchable during development. With that
>     modification, I can force off the CLMUL feature at will. And
>     voila, here is what I get (linux-x86_64):
>
>        lu0084 PrivSrc/TestCRC> /<some path>/sapjvm_9/bin/java
>     -XX:+UseNewCode TestCRC32C_OpenJDK -m
>        updateBytesCRC32C: pclmulqdq = true.
>        testing 1050 cases ...
>        lu0084 PrivSrc/TestCRC> /<some path>/sapjvm_9/bin/java
>     -XX:-UseNewCode TestCRC32C_OpenJDK -m
>        updateBytesCRC32C: pclmulqdq = false.
>        testing 1050 cases ...
>        ERROR: crc = 6f894393, crcReference = 2cdf6e8f
>        iteration 0: offsets[0] = 0  sizes[30]   = 1024
>        Exception: java.lang.Exception: TestCRC32C Error
>        Exception in thread "main" java.lang.Exception:
>     java.lang.Exception: TestCRC32C Error
>                at
>     TestCRC32C_OpenJDK.test_multi(TestCRC32C_OpenJDK.java:306)
>                at TestCRC32C_OpenJDK.main(TestCRC32C_OpenJDK.java:74)
>        Caused by: java.lang.Exception: TestCRC32C Error
>                at
>     TestCRC32C_OpenJDK.test_multi(TestCRC32C_OpenJDK.java:298)
>                ... 1 more
>        lu0084 PrivSrc/TestCRC>
>
>     Output on MacOS is the same. I did not try solaris-x86_64.
>
>     The bug is in MacroAssembler::crc32c_ipl_alg2_alt2() or one of its
>     callees. I have not yet performed an in-depth analysis on what’s
>     going wrong, mainly because I am not a proven expert on x86
>     machine code.
>
>     So how do we proceed from here? Shouldn’t we file a separate bug
>     for this x86 issue? Should this webrev (8176580) be split up into
>     the (ppc, s390) chunk and a separate test bug webrev? Can you push
>     this webrev despite there is an (unrelated) JPRT failure?
>
>     Thanks and best regards,
>     Lutz
>
>
>
>     On 12.04.2017, 18:36, "hotspot-compiler-dev on behalf of Schmidt,
>     Lutz" <hotspot-compiler-dev-bounces at openjdk.java.net
>     <mailto:hotspot-compiler-dev-bounces at openjdk.java.net> on behalf
>     of lutz.schmidt at sap.com <mailto:lutz.schmidt at sap.com>> wrote:
>
>         Hi Zoltan,
>
>         I’m not able to reproduce the problem as of now. I will try
>     out other (older) x86 machines tomorrow. Could be a problem in
>     fallback code when required vector instructions are not available.
>
>         Can you please find out on what H/W (CPU in particular) the
>     tests are failing?
>
>         Thanks,
>         Lutz
>
>
>         On 12.04.2017, 14:38, "Zoltán Majó" <zoltan.majo at oracle.com
>     <mailto:zoltan.majo at oracle.com>> wrote:
>
>             Hi Lutz,
>
>
>             On 04/12/2017 02:14 PM, Schmidt, Lutz wrote:
>             > Hi Zoltan,
>             >
>             > First of all: thanks for trying to push! Second: sorry
>     for the problems you ran into.
>
>             you are welcome and no problem, of course.
>
>             > I do not have an immediate explanation for the failure –
>     my dev machine is MacOS/x86_64. I will try to reproduce
>     immediately. For the time being: is there any log output that
>     could shed some light on the issue?
>
>             Please find the output below (and sorry for not including
>     it before).
>
>             Please let me know if you were able to reproduce the
>     problem (or not).
>
>             Best regards,
>
>
>             Zoltan
>
>             command: main -Xbatch compiler.intrinsics.zip.TestCRC32C -m
>             reason: User specified action: run
>     main/othervm/timeout=600 -Xbatch compiler.intrinsics.zip.TestCRC32C -m
>             Mode: othervm [/othervm specified]
>             elapsed time (seconds): 1.915
>             configuration:
>             STDOUT:
>             testing 1050 cases ...
>             STDERR:
>             ERROR: crc = 6f894393, crcReference = 2cdf6e8f
>             java.lang.Exception: TestCRC32C Error
>                     at
>     compiler.intrinsics.zip.TestCRC32C.check(TestCRC32C.java:218)
>                     at
>     compiler.intrinsics.zip.TestCRC32C.test_multi(TestCRC32C.java:280)
>                     at
>     compiler.intrinsics.zip.TestCRC32C.main(TestCRC32C.java:74)
>                     at
>     java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native
>     Method)
>                     at
>     java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
>                     at
>     java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
>                     at
>     java.base/java.lang.reflect.Method.invoke(Method.java:563)
>                     at
>     com.sun.javatest.regtest.agent.MainWrapper$MainThread.run(MainWrapper.java:110)
>                     at java.base/java.lang.Thread.run(Thread.java:844)
>
>             JavaTest Message: Test threw exception:
>     java.lang.Exception: TestCRC32C Error
>             JavaTest Message: shutting down test
>
>             STATUS:Failed.`main' threw exception: java.lang.Exception:
>     TestCRC32C Error
>
>
>
>             >
>             > Thanks,
>             > Lutz
>             >
>             >
>             > On 12.04.2017, 13:10, "hotspot-compiler-dev on behalf of
>     Zoltán Majó" <hotspot-compiler-dev-bounces at openjdk.java.net
>     <mailto:hotspot-compiler-dev-bounces at openjdk.java.net> on behalf
>     of zoltan.majo at oracle.com <mailto:zoltan.majo at oracle.com>> wrote:
>             >
>             >      P.S.: Forgot to mention: The problem does not
>     appear on any other x86_64
>             >      platform.
>             >
>             >      On 04/12/2017 01:07 PM, Zoltán Majó wrote:
>             >      > Hi Volker,
>             >      > Hi Lutz,
>             >      >
>             >      >
>             >      > yesterday I tried to push webrev.03 using JPRT.
>     Unfortunately, the
>             >      > TestCRC32C.java test you've modified fails on Mac
>     OS X on x86_64. Do
>             >      > you have an idea why that could be?
>             >      >
>             >      > Thank you! Best regards,
>             >      >
>             >      >
>             >      > Zoltan
>             >      >
>             >      > On 04/11/2017 06:03 PM, Volker Simonis wrote:
>             >      >> Thanks a lot Zoltan!
>             >      >>
>             >      >>
>             >      >> On Tue, Apr 11, 2017 at 4:35 PM, Zoltán Majó
>     <zoltan.majo at oracle.com <mailto:zoltan.majo at oracle.com>
>             >      >> <mailto:zoltan.majo at oracle.com
>     <mailto:zoltan.majo at oracle.com>>> wrote:
>             >      >>
>             >      >>     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>
>             >      >>         <mailto:volker.simonis at gmail.com
>     <mailto:volker.simonis at gmail.com>>
>             >      >>         <mailto:volker.simonis at gmail.com
>     <mailto:volker.simonis at gmail.com>
>             >      >>         <mailto: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>
>             >      >>         <mailto:lutz.schmidt at sap.com
>     <mailto:lutz.schmidt at sap.com>> <mailto:lutz.schmidt at sap.com
>     <mailto:lutz.schmidt at sap.com>
>             >      >>         <mailto:lutz.schmidt at sap.com
>     <mailto:lutz.schmidt at sap.com>>>>
>             >      >>         Cc: Andrew Haley <aph at redhat.com
>     <mailto:aph at redhat.com> <mailto:aph at redhat.com
>     <mailto:aph at redhat.com>>
>             >      >>         <mailto:aph at redhat.com
>     <mailto:aph at redhat.com> <mailto:aph at redhat.com
>     <mailto:aph at redhat.com>>>>,
>             >      >>         "hotspot-compiler-dev at openjdk.java.net
>     <mailto:hotspot-compiler-dev at openjdk.java.net>
>             >      >>       
>      <mailto:hotspot-compiler-dev at openjdk.java.net
>     <mailto:hotspot-compiler-dev at openjdk.java.net>>
>             >      >>       
>      <mailto:hotspot-compiler-dev at openjdk.java.net
>     <mailto:hotspot-compiler-dev at openjdk.java.net>
>             >      >> <mailto: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>
>             >      >>       
>      <mailto:hotspot-compiler-dev at openjdk.java.net
>     <mailto:hotspot-compiler-dev at openjdk.java.net>>
>             >      >>       
>      <mailto:hotspot-compiler-dev at openjdk.java.net
>     <mailto:hotspot-compiler-dev at openjdk.java.net>
>             >      >> <mailto: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> <mailto:volker.simonis at gmail.com
>     <mailto:volker.simonis at gmail.com>>
>             >      >>         <mailto:volker.simonis at gmail.com
>     <mailto:volker.simonis at gmail.com>
>             >      >>         <mailto: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> <mailto:lutz.schmidt at sap.com
>     <mailto:lutz.schmidt at sap.com>>
>             >      >>         <mailto:lutz.schmidt at sap.com
>     <mailto:lutz.schmidt at sap.com> <mailto: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/>
>             >      >>
>     <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.03/
>     <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.03/>>
>             >      >>
>     <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.03/
>     <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.03/>
>             >      >>
>     <http://cr.openjdk.java.net/%7Elucy/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> <mailto:volker.simonis at gmail.com
>     <mailto:volker.simonis at gmail.com>>
>             >      >>         <mailto:volker.simonis at gmail.com
>     <mailto:volker.simonis at gmail.com>
>             >      >>         <mailto: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> <mailto:lutz.schmidt at sap.com
>     <mailto:lutz.schmidt at sap.com>>
>             >      >>         <mailto:lutz.schmidt at sap.com
>     <mailto:lutz.schmidt at sap.com> <mailto: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/>
>             >      >>
>     <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.02/
>     <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.02/>>
>             >      >>
>     <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.02/
>     <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.02/>
>             >      >>
>     <http://cr.openjdk.java.net/%7Elucy/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:%286227%29%207-42834>
>     <tel:%2B49%20%286227%29%207-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> <mailto:volker.simonis at gmail.com
>     <mailto:volker.simonis at gmail.com>>
>             >      >>         <mailto:volker.simonis at gmail.com
>     <mailto:volker.simonis at gmail.com>
>             >      >>         <mailto: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> <mailto:lutz.schmidt at sap.com
>     <mailto:lutz.schmidt at sap.com>>
>             >      >>         <mailto:lutz.schmidt at sap.com
>     <mailto:lutz.schmidt at sap.com> <mailto: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/>
>             >      >>
>     <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.01/
>     <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.01/>>
>             >      >>
>     <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.01/
>     <http://cr.openjdk.java.net/%7Elucy/webrevs/8176580.01/>
>             >      >>
>     <http://cr.openjdk.java.net/%7Elucy/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> <mailto:volker.simonis at gmail.com
>     <mailto:volker.simonis at gmail.com>>
>             >      >>         <mailto:volker.simonis at gmail.com
>     <mailto:volker.simonis at gmail.com>
>             >      >>         <mailto: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> <mailto:aph at redhat.com
>     <mailto:aph at redhat.com>>
>             >      >>         <mailto:aph at redhat.com
>     <mailto:aph at redhat.com> <mailto: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