RFR (S) 8176580: [ppc, s390] CRC32C: wrong checksum result in some cases
Zoltán Majó
zoltan.majo at oracle.com
Thu Apr 13 14:28:31 UTC 2017
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