RFR (S) 8176580: [ppc, s390] CRC32C: wrong checksum result in some cases
Schmidt, Lutz
lutz.schmidt at sap.com
Thu Apr 13 15:17:28 UTC 2017
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.
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