RFR (S) 8176580: [ppc, s390] CRC32C: wrong checksum result in some cases
Zoltán Majó
zoltan.majo at oracle.com
Tue Apr 18 10:21:57 UTC 2017
Hi Lutz,
On 04/13/2017 05:17 PM, Schmidt, Lutz wrote:
> Hi Zoltan,
>
> I really appreciate that the enhanced tests will make their way into JDK 9!
> Re. the contributor credentials, I’m confident you will do the right thing.
> Getting the bug out of the way is by far more important.
Thank you!
Best regards,
Zoltan
>
> Regards, Lutz
>
>
>
> On 13.04.2017, 16:28, "Zoltán Majó" <zoltan.majo at oracle.com> wrote:
>
> Hi Volker,
>
>
> On 04/13/2017 04:22 PM, Volker Simonis wrote:
> > On Thu, Apr 13, 2017 at 3:39 PM, Zoltán Majó <zoltan.majo at oracle.com
> > <mailto:zoltan.majo at oracle.com>> wrote:
> >
> > 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 will only push the ppc64/s390x related fixes WITHOUT any tests
> > (actually I'm still not allowed to push shared code as it must go
> > through JPRT and there's still no access to JPRT for external
> > committers :(
>
> Thanks for clarifying.
>
> I was confused because at one place you used singular ("test") and at
> two other places you used plural ("tests"). If you were to go with (2) I
> would have offered my help with sponsoring (that's the only way I can
> help you at the moment, unfortunately).
>
> I'll take care of both tests then.
>
> Best regards,
>
>
> Zoltan
>
>
> >
> > 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).
> >
> >
> > That's fine. Thanks!
> >
> > Best regards,
> >
> >
> > Zoltan
> >
> >
> > Thank you and best regards,
> > Volker
> >
> > [1] https://bugs.openjdk.java.net/browse/JDK-8178720
> > <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>
> > <mailto: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>
> > <mailto: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>
> > <mailto: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>
> > <mailto: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.Me
> > <http://java.lang.reflect.Me>thod.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>
> > <mailto: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>
> > <mailto: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>>
> > > >> <mailto: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>>>
> > > >> <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
> > <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>>> <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 <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>>>
> > > >> <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 <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>>>
> > > >>
> > <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
> > <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>>>
> > > >>
> > <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
> > <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>>>
> > > >> <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
> > <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>>>
> > > >> <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 <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/>>>
> > > >>
> > <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/
> > <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>>>
> > > >> <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
> > <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>>>
> > > >> <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 <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/>>>
> > > >>
> > <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/
> > <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:%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>>>
> > > >> <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
> > <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>>>
> > > >> <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 <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/>>>
> > > >>
> > <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/
> > <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>>>
> > > >> <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
> > <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>>>
> > > >> <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 <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