Re: RFR: 8286462: Incorrect copyright year for src/java.base/share/classes/jdk/internal/vm/FillerObject.java
On Tue, 10 May 2022 14:58:28 GMT, zhw970623 wrote: > The copyright year of > src/java.base/share/classes/jdk/internal/vm/FillerObject.java should be 2022. Looks good and trivial. Thanks for spotting and fixing it. - Marked as reviewed by jiefu (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8633
Withdrawn: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Sun, 17 Apr 2022 14:35:14 GMT, Jie Fu wrote:
> Hi all,
>
> According to the Vector API doc, the `LSHR` operator computes
> `a>>>(n&(ESIZE*8-1))`.
> However, current implementation is incorrect for negative bytes/shorts.
>
> The background is that one of our customers try to vectorize `urshift` with
> `urshiftVector` like the following.
>
> 13 public static void urshift(byte[] src, byte[] dst) {
> 14 for (int i = 0; i < src.length; i++) {
> 15 dst[i] = (byte)(src[i] >>> 3);
> 16 }
> 17 }
> 18
> 19 public static void urshiftVector(byte[] src, byte[] dst) {
> 20 int i = 0;
> 21 for (; i < spec.loopBound(src.length); i +=spec.length()) {
> 22 var va = ByteVector.fromArray(spec, src, i);
> 23 var vb = va.lanewise(VectorOperators.LSHR, 3);
> 24 vb.intoArray(dst, i);
> 25 }
> 26
> 27 for (; i < src.length; i++) {
> 28 dst[i] = (byte)(src[i] >>> 3);
> 29 }
> 30 }
>
>
> Unfortunately and to our surprise, code@line28 computes different results
> with code@line23.
> It took quite a long time to figure out this bug.
>
> The root cause is that current implemenation of Vector API can't compute the
> unsigned right shift results as what is done for scalar `>>>` for negative
> byte/short elements.
> Actually, current implementation will do `(a & 0xFF) >>> (n & 7)` [1] for all
> bytes, which is unable to compute the vectorized `>>>` for negative bytes.
> So this seems unreasonable and unfriendly to Java developers.
> It would be better to fix it.
>
> The key idea to support unsigned right shift of negative bytes/shorts is just
> to replace the unsigned right shift operation with the signed right shift
> operation.
> This logic is:
> - For byte elements, unsigned right shift is equal to signed right shift if
> the shift_cnt <= 24.
> - For short elements, unsigned right shift is equal to signed right shift if
> the shift_cnt <= 16.
> - For Vector API, the shift_cnt will be masked to shift_cnt <= 7 for bytes
> and shift_cnt <= 15 for shorts.
>
> I just learned this idea from https://github.com/openjdk/jdk/pull/7979 .
> And many thanks to @fg1417 .
>
>
> Thanks.
> Best regards,
> Jie
>
> [1]
> https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/ByteVector.java#L935
This pull request has been closed without being integrated.
-
PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Wed, 27 Apr 2022 09:13:34 GMT, Jie Fu wrote:
>> Hi all,
>>
>> According to the Vector API doc, the `LSHR` operator computes
>> `a>>>(n&(ESIZE*8-1))`.
>> However, current implementation is incorrect for negative bytes/shorts.
>>
>> The background is that one of our customers try to vectorize `urshift` with
>> `urshiftVector` like the following.
>>
>> 13 public static void urshift(byte[] src, byte[] dst) {
>> 14 for (int i = 0; i < src.length; i++) {
>> 15 dst[i] = (byte)(src[i] >>> 3);
>> 16 }
>> 17 }
>> 18
>> 19 public static void urshiftVector(byte[] src, byte[] dst) {
>> 20 int i = 0;
>> 21 for (; i < spec.loopBound(src.length); i +=spec.length()) {
>> 22 var va = ByteVector.fromArray(spec, src, i);
>> 23 var vb = va.lanewise(VectorOperators.LSHR, 3);
>> 24 vb.intoArray(dst, i);
>> 25 }
>> 26
>> 27 for (; i < src.length; i++) {
>> 28 dst[i] = (byte)(src[i] >>> 3);
>> 29 }
>> 30 }
>>
>>
>> Unfortunately and to our surprise, code@line28 computes different results
>> with code@line23.
>> It took quite a long time to figure out this bug.
>>
>> The root cause is that current implemenation of Vector API can't compute the
>> unsigned right shift results as what is done for scalar `>>>` for negative
>> byte/short elements.
>> Actually, current implementation will do `(a & 0xFF) >>> (n & 7)` [1] for
>> all bytes, which is unable to compute the vectorized `>>>` for negative
>> bytes.
>> So this seems unreasonable and unfriendly to Java developers.
>> It would be better to fix it.
>>
>> The key idea to support unsigned right shift of negative bytes/shorts is
>> just to replace the unsigned right shift operation with the signed right
>> shift operation.
>> This logic is:
>> - For byte elements, unsigned right shift is equal to signed right shift if
>> the shift_cnt <= 24.
>> - For short elements, unsigned right shift is equal to signed right shift if
>> the shift_cnt <= 16.
>> - For Vector API, the shift_cnt will be masked to shift_cnt <= 7 for bytes
>> and shift_cnt <= 15 for shorts.
>>
>> I just learned this idea from https://github.com/openjdk/jdk/pull/7979 .
>> And many thanks to @fg1417 .
>>
>>
>> Thanks.
>> Best regards,
>> Jie
>>
>> [1]
>> https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/ByteVector.java#L935
>
>> > > According to the Vector API doc, the LSHR operator computes
>> > > a>>>(n&(ESIZE*8-1))
>>
>> Documentation is correct if viewed strictly in context of subword vector
>> lane, JVM internally promotes/sign extends subword type scalar variables
>> into int type, but vectors are loaded from continuous memory holding
>> subwords, it will not be correct for developer to imagine that individual
>> subword type lanes will be upcasted into int lanes before being operated
>> upon.
>>
>> Thus both java implementation and compiler handling looks correct.
>
> Thanks @jatin-bhateja for taking a look at this.
> After the discussion, I think it's fine to keep the current implementation of
> LSHR.
> So we're now fixing the misleading doc here:
> https://github.com/openjdk/jdk/pull/8291 .
>
> And I think it would be better to add one more operator for `>>>`.
> Thanks.
> @DamonFool should this PR and the JBS issue be closed?
Okay.
Let's close it.
-
PR: https://git.openjdk.java.net/jdk/pull/8276
Integrated: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold
On Fri, 13 May 2022 02:43:55 GMT, Jie Fu wrote: > Hi all, > > Some tests fail with Shenandoah GC after JDK-8282191. > The reason is that the assert in `ShenandoahControlThread::request_gc` misses > the case of `GCCause::_codecache_GC_threshold`. > It would be better to fix it. > > Thanks. > Best regards, > Jie This pull request has now been integrated. Changeset: 9eb15c9b Author:Jie Fu URL: https://git.openjdk.java.net/jdk/commit/9eb15c9b100b87e332c572bbc24a818e1cceb180 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold Reviewed-by: zgu - PR: https://git.openjdk.java.net/jdk/pull/8691
Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold [v2]
On Fri, 13 May 2022 12:27:16 GMT, Zhengyu Gu wrote: > LGTM Thanks @zhengyu123 for the review. - PR: https://git.openjdk.java.net/jdk/pull/8691
Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold [v2]
> Hi all, > > Some tests fail with Shenandoah GC after JDK-8282191. > The reason is that the assert in `ShenandoahControlThread::request_gc` misses > the case of `GCCause::_codecache_GC_threshold`. > It would be better to fix it. > > Thanks. > Best regards, > Jie Jie Fu has updated the pull request incrementally with one additional commit since the last revision: Revert the test change - Changes: - all: https://git.openjdk.java.net/jdk/pull/8691/files - new: https://git.openjdk.java.net/jdk/pull/8691/files/8f094d32..dc96246c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8691=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8691=00-01 Stats: 15 lines in 1 file changed: 0 ins; 15 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8691.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8691/head:pull/8691 PR: https://git.openjdk.java.net/jdk/pull/8691
Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold [v2]
On Fri, 13 May 2022 13:18:55 GMT, David Holmes wrote: >>> I think I agree with @AlanBateman - in the sense that this seems to go down >>> a slippery slope where every test would need to be executed against all >>> possible GCs. AFAIK, there are no other foreign tests doing this. >> >> I think it's a waste of time to write a separate test for this bug. >> >> I can't understand why you are against adding one more test config in the >> foreign test. >> Yes, it caught the bug in ShenandoahGC but who knows it wouldn't find some >> other bugs in the foreign api in the future. >> If you are still against the newly added test config, I can revert the test >> change. >> Thanks. > > My comment seems to have never made it through. The problem with what your > are doing is two-fold: > 1. When running the test suite specifically to test xGC for whatever x you > are now forcing a test run for Shenandoah. > 2. When x is Shenandoah then you run this test twice. > > You don't need a config just to run this with Shenandoah - you run the test > suite with Shenandoah. The test change had been reverted. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8691
Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold
On Fri, 13 May 2022 06:56:23 GMT, Alan Bateman wrote: >> Without `-XX:+UseShenandoahGC`, this bug wouldn't be exposed. >> >> What do you mean by `if you are testing with +ShenandoahGC then it will run >> already`? > > I assume you are running the tests with: >make run-tests TEST_OPTS_JAVA_OPTIONS="-XX:+UseShenandoahGC" > in which case, all of the tests you select to run will be run with that GC. > > What you have is not wrong but wouldn't be a better to add a new test to > test/hotspot/jtreg/gc rather than relying on test in test/jdk/java/foreign? > I think I agree with @AlanBateman - in the sense that this seems to go down a > slippery slope where every test would need to be executed against all > possible GCs. AFAIK, there are no other foreign tests doing this. I think it's a waste of time to write a separate test for this bug. I can't understand why you are against adding one more test config in the foreign test. Yes, it caught the bug in ShenandoahGC but who knows it wouldn't find some other bugs in the foreign api in the future. If you are still against the newly added test config, I can revert the test change. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8691
Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold
On Fri, 13 May 2022 06:56:23 GMT, Alan Bateman wrote: > I assume you are running the tests with: make run-tests > TEST_OPTS_JAVA_OPTIONS="-XX:+UseShenandoahGC" in which case, all of the tests > you select to run will be run with that GC. Yes, you're right. > What you have is not wrong but wouldn't be a better to add a new test to > test/hotspot/jtreg/gc rather than relying on test in test/jdk/java/foreign? I would suggest reusing the existing test in java/foreign. This is because this bug was first triggered after `Implementation of Foreign Function & Memory API (Preview)` integration. And I don't want a copy-paste code duplication in a new test file. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8691
Re: RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold
On Fri, 13 May 2022 06:36:42 GMT, Alan Bateman wrote: >> Hi all, >> >> Some tests fail with Shenandoah GC after JDK-8282191. >> The reason is that the assert in `ShenandoahControlThread::request_gc` >> misses the case of `GCCause::_codecache_GC_threshold`. >> It would be better to fix it. >> >> Thanks. >> Best regards, >> Jie > > test/jdk/java/foreign/TestIntrinsics.java line 48: > >> 46: * -XX:+UseShenandoahGC >> 47: * TestIntrinsics >> 48: */ > > Is this needed? The parameters looks the same as the first test description > so if you are testing with +ShenandoahGC then it will run already. Without `-XX:+UseShenandoahGC`, this bug wouldn't be exposed. What do you mean by `if you are testing with +ShenandoahGC then it will run already`? - PR: https://git.openjdk.java.net/jdk/pull/8691
RFR: 8286681: ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold
Hi all, Some tests fail with Shenandoah GC after JDK-8282191. The reason is that the assert in `ShenandoahControlThread::request_gc` misses the case of `GCCause::_codecache_GC_threshold`. It would be better to fix it. Thanks. Best regards, Jie - Commit messages: - ShenandoahControlThread::request_gc misses the case of GCCause::_codecache_GC_threshold Changes: https://git.openjdk.java.net/jdk/pull/8691/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8691=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8286681 Stats: 17 lines in 2 files changed: 17 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8691.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8691/head:pull/8691 PR: https://git.openjdk.java.net/jdk/pull/8691
Integrated: 8284992: Fix misleading Vector API doc for LSHR operator
On Tue, 19 Apr 2022 08:41:50 GMT, Jie Fu wrote: > Hi all, > > The Current Vector API doc for `LSHR` is > > Produce a>>>(n&(ESIZE*8-1)). Integral only. > > > This is misleading which may lead to bugs for Java developers. > This is because for negative byte/short elements, the results computed by > `LSHR` will be different from that of `>>>`. > For more details, please see > https://github.com/openjdk/jdk/pull/8276#issue-1206391831 . > > After the patch, the doc for `LSHR` is > > Produce zero-extended right shift of a by (n&(ESIZE*8-1)) bits. Integral only. > > > Thanks. > Best regards, > Jie This pull request has now been integrated. Changeset: e54f26aa Author:Jie Fu URL: https://git.openjdk.java.net/jdk/commit/e54f26aa3d5d44264e052bc51d3d819a8da5d1e7 Stats: 4 lines in 1 file changed: 2 ins; 0 del; 2 mod 8284992: Fix misleading Vector API doc for LSHR operator Reviewed-by: psandoz - PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator [v3]
On Thu, 28 Apr 2022 19:48:18 GMT, Paul Sandoz wrote: >> Jie Fu has updated the pull request with a new target base due to a merge or >> a rebase. The incremental webrev excludes the unrelated changes brought in >> by the merge/rebase. The pull request contains six additional commits since >> the last revision: >> >> - Address review comments >> - Merge branch 'master' into JDK-8284992 >> - Merge branch 'master' into JDK-8284992 >> - Address review comments >> - Merge branch 'master' into JDK-8284992 >> - 8284992: Fix misleading Vector API doc for LSHR operator > > It should be possible for you finalize now. Thanks @PaulSandoz for the review and help. - PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator [v4]
> Hi all, > > The Current Vector API doc for `LSHR` is > > Produce a>>>(n&(ESIZE*8-1)). Integral only. > > > This is misleading which may lead to bugs for Java developers. > This is because for negative byte/short elements, the results computed by > `LSHR` will be different from that of `>>>`. > For more details, please see > https://github.com/openjdk/jdk/pull/8276#issue-1206391831 . > > After the patch, the doc for `LSHR` is > > Produce zero-extended right shift of a by (n&(ESIZE*8-1)) bits. Integral only. > > > Thanks. > Best regards, > Jie Jie Fu has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision: - Address CSR review comments - Merge branch 'master' into JDK-8284992 - Address review comments - Merge branch 'master' into JDK-8284992 - Merge branch 'master' into JDK-8284992 - Address review comments - Merge branch 'master' into JDK-8284992 - 8284992: Fix misleading Vector API doc for LSHR operator - Changes: - all: https://git.openjdk.java.net/jdk/pull/8291/files - new: https://git.openjdk.java.net/jdk/pull/8291/files/7e82e721..0161571b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8291=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8291=02-03 Stats: 6657 lines in 233 files changed: 5591 ins; 490 del; 576 mod Patch: https://git.openjdk.java.net/jdk/pull/8291.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8291/head:pull/8291 PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator [v3]
On Thu, 28 Apr 2022 19:48:18 GMT, Paul Sandoz wrote: >> Jie Fu has updated the pull request with a new target base due to a merge or >> a rebase. The incremental webrev excludes the unrelated changes brought in >> by the merge/rebase. The pull request contains six additional commits since >> the last revision: >> >> - Address review comments >> - Merge branch 'master' into JDK-8284992 >> - Merge branch 'master' into JDK-8284992 >> - Address review comments >> - Merge branch 'master' into JDK-8284992 >> - 8284992: Fix misleading Vector API doc for LSHR operator > > It should be possible for you finalize now. Hi @PaulSandoz , the CSR had been approved and I pushed one more commit to address the CSR review comments. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator [v3]
On Thu, 28 Apr 2022 19:48:18 GMT, Paul Sandoz wrote: > It should be possible for you finalize now. Done. Thanks @PaulSandoz . - PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator [v3]
On Thu, 28 Apr 2022 00:08:41 GMT, Paul Sandoz wrote: > I created one, filled it in, and assigned it to you (for other examples you > can search in the issue tracker, this one quite is simple so i thought it was > quicker to do myself to show you). For any specification change we need to > review and track that change (independent of any implementation changes, if > any). > > If you are ok with it I can add myself as reviewer, then you can "Finalize" > it (see button on same line as "Edit"), triggering a review request, from > which we may receive comments to address, and once addressed and final, it > will unblock the PR for integration. Thanks @PaulSandoz for your help. Yes, I think it's good enough. I made a small change which just adding a `(`. https://user-images.githubusercontent.com/19923746/165657177-f44f7f7d-44da-4921-a98c-5f6b9a3d9e36.png;> Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8272352: Java launcher can not parse Chinese character when system locale is set to UTF-8
On Wed, 27 Apr 2022 20:23:32 GMT, Naoto Sato wrote: > Java runtime has been detecting the Windows system locale encoding using > `GetLocaleInfo(GetSystemDefaultLCID(), LOCALE_IDEFAULTANSICODEPAGE, ...)`, > but it returns the *legacy* ANSI code page value, e.g, 1252 for US-English. > In order to detect whether the user has selected `UTF-8` as the default, the > code page has to be queried with `GetACP()`. > Also, the case if the call to `GetLocaleInfo` fails changed to fall back to > `UTF-8` instead of `Cp1252`. Is it possible to write a jtreg test for this fix? Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8434
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator [v3]
On Wed, 27 Apr 2022 17:17:55 GMT, Paul Sandoz wrote: > Thanks, looks good, we will need to create a CSR. Have you done that before? No, and I don't know much about a CSR. Is there any example for a doc fix CSR to follow? Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Tue, 26 Apr 2022 17:31:40 GMT, Jatin Bhateja wrote: > > > According to the Vector API doc, the LSHR operator computes > > > a>>>(n&(ESIZE*8-1)) > > Documentation is correct if viewed strictly in context of subword vector > lane, JVM internally promotes/sign extends subword type scalar variables into > int type, but vectors are loaded from continuous memory holding subwords, it > will not be correct for developer to imagine that individual subword type > lanes will be upcasted into int lanes before being operated upon. > > Thus both java implementation and compiler handling looks correct. Thanks @jatin-bhateja for taking a look at this. After the discussion, I think it's fine to keep the current implementation of LSHR. So we're now fixing the misleading doc here: https://github.com/openjdk/jdk/pull/8291 . And I think it would be better to add one more operator for `>>>`. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator [v3]
> Hi all, > > The Current Vector API doc for `LSHR` is > > Produce a>>>(n&(ESIZE*8-1)). Integral only. > > > This is misleading which may lead to bugs for Java developers. > This is because for negative byte/short elements, the results computed by > `LSHR` will be different from that of `>>>`. > For more details, please see > https://github.com/openjdk/jdk/pull/8276#issue-1206391831 . > > After the patch, the doc for `LSHR` is > > Produce zero-extended right shift of a by (n&(ESIZE*8-1)) bits. Integral only. > > > Thanks. > Best regards, > Jie Jie Fu has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - Address review comments - Merge branch 'master' into JDK-8284992 - Merge branch 'master' into JDK-8284992 - Address review comments - Merge branch 'master' into JDK-8284992 - 8284992: Fix misleading Vector API doc for LSHR operator - Changes: - all: https://git.openjdk.java.net/jdk/pull/8291/files - new: https://git.openjdk.java.net/jdk/pull/8291/files/1c7f4584..7e82e721 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8291=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8291=01-02 Stats: 8171 lines in 245 files changed: 5201 ins; 1132 del; 1838 mod Patch: https://git.openjdk.java.net/jdk/pull/8291.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8291/head:pull/8291 PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator [v2]
On Tue, 26 Apr 2022 21:41:37 GMT, Paul Sandoz wrote:
> After talking with John here's what we think is a better approach than what I
> originally had in mind:
>
> 1. In the class doc of `VectorOperators` add a definition for `EMASK`
> occurring after the definition for `ESIZE`:
>
> ```
> * {@code EMASK} the bit mask of the operand type, where {@code
> EMASK=(1< ```
>
> 2. Change `LSHR` to be:
>
> ```
> /** Produce {@code (a)>>>(n&(ESIZE*8-1))}. Integral only. */
> ```
>
> That more clearly gets across operating in the correct domain for sub-word
> operand types, which was the original intention (e.g. the right shift value).
Good suggestion!
This makes sense to me.
Updated.
Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator
On Wed, 20 Apr 2022 17:24:56 GMT, Paul Sandoz wrote:
>> Hi all,
>>
>> The Current Vector API doc for `LSHR` is
>>
>> Produce a>>>(n&(ESIZE*8-1)). Integral only.
>>
>>
>> This is misleading which may lead to bugs for Java developers.
>> This is because for negative byte/short elements, the results computed by
>> `LSHR` will be different from that of `>>>`.
>> For more details, please see
>> https://github.com/openjdk/jdk/pull/8276#issue-1206391831 .
>>
>> After the patch, the doc for `LSHR` is
>>
>> Produce zero-extended right shift of a by (n&(ESIZE*8-1)) bits. Integral
>> only.
>>
>>
>> Thanks.
>> Best regards,
>> Jie
>
> We can raise attention to that:
>
> /** Produce {@code a>>>(n&(ESIZE*8-1))}
> * (The operand and result are converted if the operand type is {@code byte}
> or {@code short}, see below). Integral only.
> * ...
> */
Hi @PaulSandoz ,
I add a piece of notice at the end of the brief description of `LSHR` since not
everyone would click and see the details without the change.
What do you think?
Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator
On Wed, 20 Apr 2022 17:24:56 GMT, Paul Sandoz wrote:
> We can raise attention to that:
>
> ```
> /** Produce {@code a>>>(n&(ESIZE*8-1))}
> * (The operand and result are converted if the operand type is {@code byte}
> or {@code short}, see below). Integral only.
> * ...
> */
> ```
It seems still misleading if we don't change the brief description of `LSHR`.
How about adding 'see details for attention' like this?
https://user-images.githubusercontent.com/19923746/164371693-6e26842c-47f4-44f5-8371-1906ae8e6218.png;>
And the patch had been updated.
Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator [v2]
> Hi all, > > The Current Vector API doc for `LSHR` is > > Produce a>>>(n&(ESIZE*8-1)). Integral only. > > > This is misleading which may lead to bugs for Java developers. > This is because for negative byte/short elements, the results computed by > `LSHR` will be different from that of `>>>`. > For more details, please see > https://github.com/openjdk/jdk/pull/8276#issue-1206391831 . > > After the patch, the doc for `LSHR` is > > Produce zero-extended right shift of a by (n&(ESIZE*8-1)) bits. Integral only. > > > Thanks. > Best regards, > Jie Jie Fu has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision: - Address review comments - Merge branch 'master' into JDK-8284992 - 8284992: Fix misleading Vector API doc for LSHR operator - Changes: - all: https://git.openjdk.java.net/jdk/pull/8291/files - new: https://git.openjdk.java.net/jdk/pull/8291/files/50235163..1c7f4584 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8291=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8291=00-01 Stats: 11427 lines in 826 files changed: 6952 ins; 1816 del; 2659 mod Patch: https://git.openjdk.java.net/jdk/pull/8291.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8291/head:pull/8291 PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator
On Tue, 19 Apr 2022 08:41:50 GMT, Jie Fu wrote: > Hi all, > > The Current Vector API doc for `LSHR` is > > Produce a>>>(n&(ESIZE*8-1)). Integral only. > > > This is misleading which may lead to bugs for Java developers. > This is because for negative byte/short elements, the results computed by > `LSHR` will be different from that of `>>>`. > For more details, please see > https://github.com/openjdk/jdk/pull/8276#issue-1206391831 . > > After the patch, the doc for `LSHR` is > > Produce zero-extended right shift of a by (n&(ESIZE*8-1)) bits. Integral only. > > > Thanks. > Best regards, > Jie Add hotspot-compiler since the JBS has been moved there. - PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Wed, 20 Apr 2022 00:25:26 GMT, Paul Sandoz wrote: > I am yet to be convinced we a 3rd vector right shift operator. We are talking > about narrow cases of correct use which i believe can be supported with the > existing operators. The user needs to think very careful when deviating from > common right shift patterns on sub-words, which when deviated from can often > imply misunderstanding and incorrect use, or an obtuse use. I would prefer to > stick the existing operators and clarify their use. As we can see, `VectorOperators` has directly supported all the unary/binary scalar operators in the Java language except for `>>>`. So it seems strange not to support `>>>` directly. Since you are a Vector API expert, you know the semantics of `LSHR` precisely. But for many Java developers, things are different. I'm afraid most of them don't know Vector API actually has extended semantics of `>>>` upon bytes/shorts with `LSHR`. To be honest, I didn't know it before my customer's bug even though I had spent enough time reading the Vector API doc. This is because for ordinary developers, they are only familiar with the common scalar `>>>`. So it seems easy to write bugs with the only `LSHR`, which is different with `>>>`. >From the developer's point of view, I strongly suggest providing the `>>>` >operator in Vector API. Not only because `>>>` is one of the basic operators in Java language, but also we can make it to be more friendly to so many ordinary developers. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator
On Wed, 20 Apr 2022 00:46:32 GMT, Paul Sandoz wrote:
> The intended pattern for the operator tokens is to present a short symbolic
> description using Java operators and common methods. It would be good to try
> and keep with this pattern, and clarify for the extra cases. Here's what i
> had in mind:
>
> ```
> /** Produce {@code a>>>(n&(ESIZE*8-1))}. Integral only.
> *
> * For operand types {@code byte} and {@code short} the operation behaves as
> if the operand is first implicitly widened
> * to an {@code int} value with {@code (a & ((1 << ESIZE) - 1))} the result
> of which is then applied as the operand to this
> * operation, the result of the operation is then narrowed from {@code int}
> to the operand type using an explicit cast.
> */
> public static final /*bitwise*/ Binary LSHR;
> ```
This works only if people would like to read the detailed description of `LSHR`
carefully.
Actually, most developers would still see the brief description first.
https://user-images.githubusercontent.com/19923746/164127620-90a73d29-868e-46d8-9562-2c5b2021f13b.png;>
If they don't click out the detailed description further or don't read it
carefully, it's still misleading.
Maybe, we'd better not to use `>>>` in the brief description since `LSHR`
behaves differently with the common used `>>>`.
What do you think?
-
PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator
On Tue, 19 Apr 2022 16:11:37 GMT, Paul Sandoz wrote: > I need to think a little more about this. The specification is not accurate > and likely requires a CSR. > > My initial thoughts are I would prefer the operation to retain reference to > the succinct definition using the logical right shift operator but we add > additional specification explaining the cases for `byte` and `short`, namely > that the result is widened to an `int` as if by `(a & ((1 << ESIZE) - 1))` > and the result narrowed. That's commonly (at least for the widening part) how > `>>>` is used with `byte` and `short`, and i think would be clearer to be > explicit in that regard. OK. To avoid confusing I would prefer using the description of `>>>` in the new vector operator, which would perform the same behavior as the scalar `>>>`. - PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Tue, 19 Apr 2022 17:40:07 GMT, Paul Sandoz wrote: > Not yet talked with John, but i investigated further. The implementation of > the `LSHR` operation is behaving as intended, but is under specified with > regards to `byte` and `short` as you noted in #8291. > > This is a subtle area, but i am wondering if the user really means to use > arithmetic shift in this case? Since is not the following true for all values > of `e` and `c`, where `e` is a `byte` and `c` is the right shift count > ranging from 0 to 7: > > ``` > (byte) (e >>> c) == (byte) (e >> c) > ``` > > ? > > Then the user can use `VectorOperators.ASHR`. Yes, in theory, the user can use `ASHR`. But people have to be very careful about when to use `AHSR` and when to use `LSHR`, which is really inconvenient and easy to make a mistake. And not all the people are smart enough to know this skill for bytes/shorts. So to make it to be programmed more easily and also reduce the possibility to make mistakes, a new operator for scalar `>>>` would be helpful when vectorizing with Vector API. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Sun, 17 Apr 2022 14:35:14 GMT, Jie Fu wrote:
> Hi all,
>
> According to the Vector API doc, the `LSHR` operator computes
> `a>>>(n&(ESIZE*8-1))`.
> However, current implementation is incorrect for negative bytes/shorts.
>
> The background is that one of our customers try to vectorize `urshift` with
> `urshiftVector` like the following.
>
> 13 public static void urshift(byte[] src, byte[] dst) {
> 14 for (int i = 0; i < src.length; i++) {
> 15 dst[i] = (byte)(src[i] >>> 3);
> 16 }
> 17 }
> 18
> 19 public static void urshiftVector(byte[] src, byte[] dst) {
> 20 int i = 0;
> 21 for (; i < spec.loopBound(src.length); i +=spec.length()) {
> 22 var va = ByteVector.fromArray(spec, src, i);
> 23 var vb = va.lanewise(VectorOperators.LSHR, 3);
> 24 vb.intoArray(dst, i);
> 25 }
> 26
> 27 for (; i < src.length; i++) {
> 28 dst[i] = (byte)(src[i] >>> 3);
> 29 }
> 30 }
>
>
> Unfortunately and to our surprise, code@line28 computes different results
> with code@line23.
> It took quite a long time to figure out this bug.
>
> The root cause is that current implemenation of Vector API can't compute the
> unsigned right shift results as what is done for scalar `>>>` for negative
> byte/short elements.
> Actually, current implementation will do `(a & 0xFF) >>> (n & 7)` [1] for all
> bytes, which is unable to compute the vectorized `>>>` for negative bytes.
> So this seems unreasonable and unfriendly to Java developers.
> It would be better to fix it.
>
> The key idea to support unsigned right shift of negative bytes/shorts is just
> to replace the unsigned right shift operation with the signed right shift
> operation.
> This logic is:
> - For byte elements, unsigned right shift is equal to signed right shift if
> the shift_cnt <= 24.
> - For short elements, unsigned right shift is equal to signed right shift if
> the shift_cnt <= 16.
> - For Vector API, the shift_cnt will be masked to shift_cnt <= 7 for bytes
> and shift_cnt <= 15 for shorts.
>
> I just learned this idea from https://github.com/openjdk/jdk/pull/7979 .
> And many thanks to @fg1417 .
>
>
> Thanks.
> Best regards,
> Jie
>
> [1]
> https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/ByteVector.java#L935
We plan to fix the doc: https://github.com/openjdk/jdk/pull/8291 first.
Then, let's see what @PaulSandoz would think of adding a new operator for the
scalar `>>>`.
Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/8276
RFR: 8284992: Fix misleading Vector API doc for LSHR operator
Hi all, The Current Vector API doc for `LSHR` is Produce a>>>(n&(ESIZE*8-1)). Integral only. This is misleading which may lead to bugs for Java developers. This is because for negative byte/short elements, the results computed by `LSHR` will be different from that of `>>>`. For more details, please see https://github.com/openjdk/jdk/pull/8276#issue-1206391831 . After the patch, the doc for `LSHR` is Produce zero-extended right shift of a by (n&(ESIZE*8-1)) bits. Integral only. Thanks. Best regards, Jie - Commit messages: - 8284992: Fix misleading Vector API doc for LSHR operator Changes: https://git.openjdk.java.net/jdk/pull/8291/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8291=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284992 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8291.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8291/head:pull/8291 PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Tue, 19 Apr 2022 02:43:33 GMT, Quan Anh Mai wrote: > I see, however, I preserve the opinion that the API doc implies the extended > unsigned right shift not the original `>>>` (or the output types would be > wrong). So, I think you can create another operator that perform the scalar > `>>>` if it is needed. > > Thank you very much. Thanks @merykitty for your understanding. After the discussion, I got the point that the original implementation of `LSHR` for bytes/shorts is useful and needed. So let's just keep it. Yes, we think the operator for scalar `>>>` is needed for several reasons: 1. We do have scalar `>>>` upon bytes/shorts in real programs. 2. There is usually no guarantee that all the operands would be non-negative for `>>>`. 3. Make it to be programmed more easily and also reduce the possibility to make mistakes. Java developers would be happy and appreciated with that operator I believe. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Mon, 18 Apr 2022 08:29:52 GMT, Jie Fu wrote:
>>> @DamonFool
>>>
>>> I think the issue is that these two cases of yours are not equal
>>> semantically.
>>
>> Why?
>> According to the vector api doc, they should compute the same value when the
>> shift_cnt is 3, right?
>>
>>>
>>> ```
>>> 13 public static void urshift(byte[] src, byte[] dst) {
>>> 14 for (int i = 0; i < src.length; i++) {
>>> 15 dst[i] = (byte)(src[i] >>> 3);
>>> 16 }
>>> 17 }
>>> 18
>>> 19 public static void urshiftVector(byte[] src, byte[] dst) {
>>> 20 int i = 0;
>>> 21 for (; i < spec.loopBound(src.length); i +=spec.length()) {
>>> 22 var va = ByteVector.fromArray(spec, src, i);
>>> 23 var vb = va.lanewise(VectorOperators.LSHR, 3);
>>> 24 vb.intoArray(dst, i);
>>> 25 }
>>> 26
>>> 27 for (; i < src.length; i++) {
>>> 28 dst[i] = (byte)(src[i] >>> 3);
>>> 29 }
>>> 30 }
>>> ```
>>>
>>> Besides the unsigned shift, line15 also has a type conversion which is
>>> missing in the vector api case. To get the equivalent result, one need to
>>> cast the result explicitly at line24, e.g,
>>> `((IntVector)vb.castShape(SPECISE_XXX, 0)).intoArray(idst, i);`
>>
>> Since all the vector operations are already based on byte lane type, I don't
>> think we need a `cast` operation here.
>> Can we solve this problem by inserting a cast operation?
>
>> @DamonFool
>>
>> `>>>` can not apply to sub-word type in Java. `(byte)(src[i] >>> 3)` is
>> unsigned right shift in type of INT and transformed the result to BYTE. In
>> vector api, it extends the `>>>` to sub-word type with the same semantic
>> meaning like `iushr`[1], that is zero extending.
>>
>> > The vector api docs says it would compute a>>>(n&(ESIZE*8-1)).
>>
>> I think `>>>` has some extending meanings here. As I said above, no sub-word
>> type for `>>>` in Java.
>>
>> [1]
>> https://docs.oracle.com/javase/specs/jvms/se18/html/jvms-6.html#jvms-6.5.iushr
>
> As discussed above
> https://github.com/openjdk/jdk/pull/8276#issuecomment-1101016904 , there
> isn't any problem to apply `>>>` upon shorts/bytes.
>
> What do you think of https://github.com/openjdk/jdk/pull/7979 , which tries
> to vectorize unsigned shift right on signed subword types ?
> And what do you think of the benchmarks mentioned in that PR?
>
> The vector api doc clearly states `LSHR` operator would compute
> `a>>>(n&(ESIZE*8-1))`.
> But it fails to do so when `a` is negative byte/short element.
>
> So if the doc description is correct, the current implementation would be
> wrong, right?
>
> However, if you think the current implementation is correct, the vector api
> doc would be wrong.
> Then, we would lack an operator working like the scalar `>>>` since current
> implementation fails to do so for negative bytes/shorts.
> Hi @DamonFool, the doc does obviously not mean what you think, and actually
> seems to indicate the Eric's interpretation instead. Simply because `a >>> (n
> & (ESIZE - 1))` does not output the type of `a` for subword-type inputs,
> which is clearly wrong. This suggests that the doc here should be interpreted
> that `>>>` is the extended shift operation, which is defined on subword types
> the same as for words and double-words. Though, I agree that the doc must be
> modified to reflect the intention more clearly.
>
My intention is to make Vector API to be more friendly to Java developers.
The so called extended unsigned right shift operation for bytes/shorts actually
behave differently with the well-known scalar `>>>`, which may become the
source of bugs.
> > Then, we would lack an operator working like the scalar >>> since current
> > implementation fails to do so for negative bytes/shorts.
>
> As you have noted, we have `ASHR` for bytes, shorts and `LSHR` for ints,
> longs. Thanks a lot.
Then people have to be very careful about when to use `AHSR` and when to use
`LSHR`, which is really inconvenient and easy to make a mistake.
And not all the people are smart enough to know this skill for bytes/shorts.
So simply modifying the vector api doc can't solve these problems.
Maybe, we can add one more operator to distinguish the semantics of scalar
`>>>` with the so called extended unsigned right shift operation for
bytes/shorts.
-
PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Mon, 18 Apr 2022 05:14:25 GMT, Jie Fu wrote:
>>> > However, just image that someone would like to optimize some code
>>> > segments of bytes/shorts `>>>`
>>>
>>> Then that person can just use signed shift (`VectorOperators.ASHR`), right?
>>> Shifting on masked shift counts means that the shift count cannot be
>>> greater than 8 for bytes and 16 for shorts, which means that `(byte)(src[i]
>>> >>> 3)` is exactly the same as `(byte)(src[i] >> 3)`. Please correct me if
>>> I misunderstood something here.
>>
>> Yes, you're right that's why I said `LSHR` can be replaced with `ASHR`.
>>
>> However, not all the people are clever enough to do this source code level
>> replacement.
>> To be honest, I would never think of that `>>>` can be auto-vectorized by
>> this idea proposed by https://github.com/openjdk/jdk/pull/7979 .
>>
>>>
>>> Your proposed changes make unsigned shifts for subwords behave exactly the
>>> same as signed shifts, which is both redundant (we have 2 operations doing
>>> exactly the same thing) and inadequate (we lack the operation to do the
>>> proper unsigned shifts)
>>
>> `LSHR` following the behavior of scalar `>>>` is very important for Java
>> developers to rewrite the code with vector api.
>> Maybe, we should add one more operator to support what you called `the
>> proper unsigned shifts`, right?
>> But that's another topic which can be done in a separate issue.
>
>> @DamonFool
>>
>> I think the issue is that these two cases of yours are not equal
>> semantically.
>
> Why?
> According to the vector api doc, they should compute the same value when the
> shift_cnt is 3, right?
>
>>
>> ```
>> 13 public static void urshift(byte[] src, byte[] dst) {
>> 14 for (int i = 0; i < src.length; i++) {
>> 15 dst[i] = (byte)(src[i] >>> 3);
>> 16 }
>> 17 }
>> 18
>> 19 public static void urshiftVector(byte[] src, byte[] dst) {
>> 20 int i = 0;
>> 21 for (; i < spec.loopBound(src.length); i +=spec.length()) {
>> 22 var va = ByteVector.fromArray(spec, src, i);
>> 23 var vb = va.lanewise(VectorOperators.LSHR, 3);
>> 24 vb.intoArray(dst, i);
>> 25 }
>> 26
>> 27 for (; i < src.length; i++) {
>> 28 dst[i] = (byte)(src[i] >>> 3);
>> 29 }
>> 30 }
>> ```
>>
>> Besides the unsigned shift, line15 also has a type conversion which is
>> missing in the vector api case. To get the equivalent result, one need to
>> cast the result explicitly at line24, e.g,
>> `((IntVector)vb.castShape(SPECISE_XXX, 0)).intoArray(idst, i);`
>
> Since all the vector operations are already based on byte lane type, I don't
> think we need a `cast` operation here.
> Can we solve this problem by inserting a cast operation?
> @DamonFool
>
> `>>>` can not apply to sub-word type in Java. `(byte)(src[i] >>> 3)` is
> unsigned right shift in type of INT and transformed the result to BYTE. In
> vector api, it extends the `>>>` to sub-word type with the same semantic
> meaning like `iushr`[1], that is zero extending.
>
> > The vector api docs says it would compute a>>>(n&(ESIZE*8-1)).
>
> I think `>>>` has some extending meanings here. As I said above, no sub-word
> type for `>>>` in Java.
>
> [1]
> https://docs.oracle.com/javase/specs/jvms/se18/html/jvms-6.html#jvms-6.5.iushr
As discussed above
https://github.com/openjdk/jdk/pull/8276#issuecomment-1101016904 , there isn't
any problem to apply `>>>` upon shorts/bytes.
What do you think of https://github.com/openjdk/jdk/pull/7979 , which tries to
vectorize unsigned shift right on signed subword types ?
And what do you think of the benchmarks mentioned in that PR?
The vector api doc clearly states `LSHR` operator would compute
`a>>>(n&(ESIZE*8-1))`.
But it fails to do so when `a` is negative byte/short element.
So if the doc description is correct, the current implementation would be
wrong, right?
However, if you think the current implementation is correct, the vector api doc
would be wrong.
Then, we would lack an operator working like the scalar `>>>` since current
implementation fails to do so for negative bytes/shorts.
-
PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Mon, 18 Apr 2022 05:09:33 GMT, Jie Fu wrote:
>>> However, just image that someone would like to optimize some code segments
>>> of bytes/shorts `>>>`
>>
>> Then that person can just use signed shift (`VectorOperators.ASHR`), right?
>> Shifting on masked shift counts means that the shift count cannot be greater
>> than 8 for bytes and 16 for shorts, which means that `(byte)(src[i] >>> 3)`
>> is exactly the same as `(byte)(src[i] >> 3)`. Please correct me if I
>> misunderstood something here.
>>
>> Your proposed changes make unsigned shifts for subwords behave exactly the
>> same as signed shifts, which is both redundant (we have 2 operations doing
>> exactly the same thing) and inadequate (we lack the operation to do the
>> proper unsigned shifts)
>>
>> Thank you very much.
>
>> > However, just image that someone would like to optimize some code segments
>> > of bytes/shorts `>>>`
>>
>> Then that person can just use signed shift (`VectorOperators.ASHR`), right?
>> Shifting on masked shift counts means that the shift count cannot be greater
>> than 8 for bytes and 16 for shorts, which means that `(byte)(src[i] >>> 3)`
>> is exactly the same as `(byte)(src[i] >> 3)`. Please correct me if I
>> misunderstood something here.
>
> Yes, you're right that's why I said `LSHR` can be replaced with `ASHR`.
>
> However, not all the people are clever enough to do this source code level
> replacement.
> To be honest, I would never think of that `>>>` can be auto-vectorized by
> this idea proposed by https://github.com/openjdk/jdk/pull/7979 .
>
>>
>> Your proposed changes make unsigned shifts for subwords behave exactly the
>> same as signed shifts, which is both redundant (we have 2 operations doing
>> exactly the same thing) and inadequate (we lack the operation to do the
>> proper unsigned shifts)
>
> `LSHR` following the behavior of scalar `>>>` is very important for Java
> developers to rewrite the code with vector api.
> Maybe, we should add one more operator to support what you called `the proper
> unsigned shifts`, right?
> But that's another topic which can be done in a separate issue.
> @DamonFool
>
> I think the issue is that these two cases of yours are not equal semantically.
Why?
According to the vector api doc, they should compute the same value when the
shift_cnt is 3, right?
>
> ```
> 13 public static void urshift(byte[] src, byte[] dst) {
> 14 for (int i = 0; i < src.length; i++) {
> 15 dst[i] = (byte)(src[i] >>> 3);
> 16 }
> 17 }
> 18
> 19 public static void urshiftVector(byte[] src, byte[] dst) {
> 20 int i = 0;
> 21 for (; i < spec.loopBound(src.length); i +=spec.length()) {
> 22 var va = ByteVector.fromArray(spec, src, i);
> 23 var vb = va.lanewise(VectorOperators.LSHR, 3);
> 24 vb.intoArray(dst, i);
> 25 }
> 26
> 27 for (; i < src.length; i++) {
> 28 dst[i] = (byte)(src[i] >>> 3);
> 29 }
> 30 }
> ```
>
> Besides the unsigned shift, line15 also has a type conversion which is
> missing in the vector api case. To get the equivalent result, one need to
> cast the result explicitly at line24, e.g,
> `((IntVector)vb.castShape(SPECISE_XXX, 0)).intoArray(idst, i);`
Since all the vector operations are already based on byte lane type, I don't
think we need a `cast` operation here.
Can we solve this problem by inserting a cast operation?
-
PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Mon, 18 Apr 2022 04:25:23 GMT, Quan Anh Mai wrote: > > However, just image that someone would like to optimize some code segments > > of bytes/shorts `>>>` > > Then that person can just use signed shift (`VectorOperators.ASHR`), right? > Shifting on masked shift counts means that the shift count cannot be greater > than 8 for bytes and 16 for shorts, which means that `(byte)(src[i] >>> 3)` > is exactly the same as `(byte)(src[i] >> 3)`. Please correct me if I > misunderstood something here. Yes, you're right that's why I said `LSHR` can be replaced with `ASHR`. However, not all the people are clever enough to do this source code level replacement. To be honest, I would never think of that `>>>` can be auto-vectorized by this idea proposed by https://github.com/openjdk/jdk/pull/7979 . > > Your proposed changes make unsigned shifts for subwords behave exactly the > same as signed shifts, which is both redundant (we have 2 operations doing > exactly the same thing) and inadequate (we lack the operation to do the > proper unsigned shifts) `LSHR` following the behavior of scalar `>>>` is very important for Java developers to rewrite the code with vector api. Maybe, we should add one more operator to support what you called `the proper unsigned shifts`, right? But that's another topic which can be done in a separate issue. - PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Mon, 18 Apr 2022 03:48:13 GMT, Quan Anh Mai wrote: > Because unsigned cast should operate on unsigned types, the more appropriate > usage is `(src[i] & 0xFF) >>> 3`, with the `&` operation is the cast from > unsigned byte to int. Actually, I fail to understand the intention of your > example, why not use signed shift instead, what does unsigned shift provide > here apart from extra cognitive load in reasoning the operation. The fact is that you can't prevent developers from using `>>>` upon negative elements since neither the JVMS nor the JLS prevents it. > May you provide a more concrete example to the utilisation of unsigned shift > on signed subword types, please. The example provided by @fg1417 in #7979 > seems to indicate the real intention is to right shifting unsigned bytes, > with the unsigned cast sometimes omitted (changed to a signed cast) because > the shift results are masked by a stricter mask immediately. Sorry, I can't show the detail of our customer's code. However, just image that someone would like to optimize some code segments of bytes/shorts `>>>`, how can you say there should be always non-negative operands? - PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Sun, 17 Apr 2022 14:35:14 GMT, Jie Fu wrote:
> Hi all,
>
> According to the Vector API doc, the `LSHR` operator computes
> `a>>>(n&(ESIZE*8-1))`.
> However, current implementation is incorrect for negative bytes/shorts.
>
> The background is that one of our customers try to vectorize `urshift` with
> `urshiftVector` like the following.
>
> 13 public static void urshift(byte[] src, byte[] dst) {
> 14 for (int i = 0; i < src.length; i++) {
> 15 dst[i] = (byte)(src[i] >>> 3);
> 16 }
> 17 }
> 18
> 19 public static void urshiftVector(byte[] src, byte[] dst) {
> 20 int i = 0;
> 21 for (; i < spec.loopBound(src.length); i +=spec.length()) {
> 22 var va = ByteVector.fromArray(spec, src, i);
> 23 var vb = va.lanewise(VectorOperators.LSHR, 3);
> 24 vb.intoArray(dst, i);
> 25 }
> 26
> 27 for (; i < src.length; i++) {
> 28 dst[i] = (byte)(src[i] >>> 3);
> 29 }
> 30 }
>
>
> Unfortunately and to our surprise, code@line28 computes different results
> with code@line23.
> It took quite a long time to figure out this bug.
>
> The root cause is that current implemenation of Vector API can't compute the
> unsigned right shift results as what is done for scalar `>>>` for negative
> byte/short elements.
> Actually, current implementation will do `(a & 0xFF) >>> (n & 7)` [1] for all
> bytes, which is unable to compute the vectorized `>>>` for negative bytes.
> So this seems unreasonable and unfriendly to Java developers.
> It would be better to fix it.
>
> The key idea to support unsigned right shift of negative bytes/shorts is just
> to replace the unsigned right shift operation with the signed right shift
> operation.
> This logic is:
> - For byte elements, unsigned right shift is equal to signed right shift if
> the shift_cnt <= 24.
> - For short elements, unsigned right shift is equal to signed right shift if
> the shift_cnt <= 16.
> - For Vector API, the shift_cnt will be masked to shift_cnt <= 7 for bytes
> and shift_cnt <= 15 for shorts.
>
> I just learned this idea from https://github.com/openjdk/jdk/pull/7979 .
> And many thanks to @fg1417 .
>
>
> Thanks.
> Best regards,
> Jie
>
> [1]
> https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/ByteVector.java#L935
According to the vector api doc, `LSHR` seems to be designed to vectorize the
scalar `>>>` with masked `shift_cnt`.
Since for most cases, if we use vector api to rewrite the scalar code, we don't
know if all the inputs are positive only.
So it would be better to follow the scalar `>>>` behavior for any supported
masked `shift_cnt`.
Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Mon, 18 Apr 2022 02:32:39 GMT, Quan Anh Mai wrote: > What I try to convey here is that `src[i] >>> 3` is not a byte unsigned > shift, it is an int unsigned shift following a byte-to-int promotion. This is > different from the Vector API that has definition for the shift operations of > subword types. The vector api docs says it would compute `a>>>(n&(ESIZE*8-1))`. https://user-images.githubusercontent.com/19923746/163746064-b1c1ae37-245e-4a92-9020-99b13195193d.png;> > Also you can see the reasons in these lines of comments: > > https://github.com/openjdk/jdk/blob/e5041ae3d45b43be10d5da747d773882ebf0482b/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/ByteVector.java#L944 > > Thanks. The patch wouldn't change the masked shift count of vector api. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Sun, 17 Apr 2022 23:53:49 GMT, Quan Anh Mai wrote: > According to JLS section 5.8, any operand in a numeric arithmetic context > undergoes a promotion to int, long, float or double and the operation is only > defined for values of the promoted types. This means that `>>>` is not > defined for byte/short values and the real behaviour here is that `src[i]` > get promoted to int by a signed cast before entering the unsigned shift > operation. This is different from `VectorOperators.LSHR` which is defined for > byte/short element types. The scalar code does not do a byte unsigned shift > but an int unsigned shift between a promotion and a narrowing cast, the > explicit cast `(byte)` notifies the user of this behaviour. I can't understand why people can't use `>>>` for negative bytes/shorts. - Does the spec forbidden this usage? - Is there any compile error? - Is there any runtime error? - Is the behavior to be undefined? The JLS you mentioned actually defines how to compute `>>>` for bytes/shorts in Java, which applies to both positive and negative bytes/shorts. - First, it gets promoted. - Then, do something else. So I disagree with you if the JLS spec doesn't say people are not allowed to use `>>>` for negative bytes/shorts. > > Secondly, consistency is the key, having a byte unsigned shift promoting > elements to ints is not consistent, I can argue why aren't int elements being > promoted to longs, or longs being promoted to the 128-bit integral type, too. > Well, this kind of behavior was specified by the Java spec rules many years ago. We have to just follow these rules if you can't change the specs. > Finally, as I have mentioned in #7979, this usage of unsigned shift seems > more likely to be a bug than an intended behaviour, so we should not bother > to optimise this pattern. Since the behavior of shift operations is clearly defined by the Java specs and supported by the language, how do you know there is no one to use it intendedly? - PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Sun, 17 Apr 2022 17:25:57 GMT, Quan Anh Mai wrote: > Hi, > > The `>>>` operator is not defined for subword types, what the code in line 28 > does vs what it is supposed to do are different, which is more likely the bug > here. An unsigned shift should operate on subword types the same as it does > on word and double-word types, which is to zero extend the value before > shifting it rightwards. > > Another argument would be that an unsigned shift operates on the unsigned > types, and the signed cast exposes this misunderstanding regarding the > operation. > > Thanks. Thanks @merykitty for your comments. What I show in this PR is the typical translation of a Java scalar program to Vector API code. Obviously, the implementation is wrong for negative bytes/shorts according to the description of the Vecotor API doc. As a general programming language, Java does support the usage of `>>>` for negative bytes/shorts. Will you use this Vector API to optimize a Java lib which doesn't know the actual input at all? For a given shift_cnt, why not produce the same result for Vector API just as what is done for scalar `>>>`? - PR: https://git.openjdk.java.net/jdk/pull/8276
RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
Hi all,
According to the Vector API doc, the `LSHR` operator computes
`a>>>(n&(ESIZE*8-1))`.
However, current implementation is incorrect for negative bytes/shorts.
The background is that one of our customers try to vectorize `urshift` with
`urshiftVector` like the following.
13 public static void urshift(byte[] src, byte[] dst) {
14 for (int i = 0; i < src.length; i++) {
15 dst[i] = (byte)(src[i] >>> 3);
16 }
17 }
18
19 public static void urshiftVector(byte[] src, byte[] dst) {
20 int i = 0;
21 for (; i < spec.loopBound(src.length); i +=spec.length()) {
22 var va = ByteVector.fromArray(spec, src, i);
23 var vb = va.lanewise(VectorOperators.LSHR, 3);
24 vb.intoArray(dst, i);
25 }
26
27 for (; i < src.length; i++) {
28 dst[i] = (byte)(src[i] >>> 3);
29 }
30 }
Unfortunately and to our surprise, code@line28 computes different results with
code@line23.
It took quite a long time to figure out this bug.
The root cause is that current implemenation of Vector API can't compute the
unsigned right shift results as what is done for scalar `>>>` for negative
byte/short elements.
Actually, current implementation will do `(a & 0xFF) >>> (n & 7)` [1] for all
bytes, which is unable to compute the vectorized `>>>` for negative bytes.
So this seems unreasonable and unfriendly to Java developers.
It would be better to fix it.
The key idea to support unsigned right shift of negative bytes/shorts is just
to replace the unsigned right shift operation with the signed right shift
operation.
This logic is:
- For byte elements, unsigned right shift is equal to signed right shift if the
shift_cnt <= 24.
- For short elements, unsigned right shift is equal to signed right shift if
the shift_cnt <= 16.
- For Vector API, the shift_cnt will be masked to shift_cnt <= 7 for bytes and
shift_cnt <= 15 for shorts.
I just learned this idea from https://github.com/openjdk/jdk/pull/7979 .
And many thanks to @fg1417 .
Thanks.
Best regards,
Jie
[1]
https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/ByteVector.java#L935
-
Commit messages:
- Add a space
- 8284932: [Vector API] Incorrect implementation of LSHR operator for negative
byte/short elements
Changes: https://git.openjdk.java.net/jdk/pull/8276/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8276=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8284932
Stats: 77 lines in 17 files changed: 19 ins; 12 del; 46 mod
Patch: https://git.openjdk.java.net/jdk/pull/8276.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8276/head:pull/8276
PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8282162: [vector] Optimize integral vector negation API [v3]
On Mon, 28 Mar 2022 09:56:22 GMT, Xiaohong Gong wrote: >> The current vector `"NEG"` is implemented with substraction a vector by zero >> in case the architecture does not support the negation instruction. And to >> fit the predicate feature for architectures that support it, the masked >> vector `"NEG" ` is implemented with pattern `"v.not(m).add(1, m)"`. They >> both can be optimized to a single negation instruction for ARM SVE. >> And so does the non-masked "NEG" for NEON. Besides, implementing the masked >> "NEG" with substraction for architectures that support neither negation >> instruction nor predicate feature can also save several instructions than >> the current pattern. >> >> To optimize the VectorAPI negation, this patch moves the implementation from >> Java side to hotspot. The compiler will generate different nodes according >> to the architecture: >> - Generate the (predicated) negation node if architecture supports it, >> otherwise, generate "`zero.sub(v)`" pattern for non-masked operation. >> - Generate `"zero.sub(v, m)"` for masked operation if the architecture >> does not have predicate feature, otherwise generate the original pattern >> `"v.xor(-1, m).add(1, m)"`. >> >> So with this patch, the following transformations are applied: >> >> For non-masked negation with NEON: >> >> moviv16.4s, #0x0 >> sub v17.4s, v16.4s, v17.4s ==> neg v17.4s, v17.4s >> >> and with SVE: >> >> mov z16.s, #0 >> sub z18.s, z16.s, z17.s ==> neg z16.s, p7/m, z16.s >> >> For masked negation with NEON: >> >> moviv17.4s, #0x1 >> mvn v19.16b, v18.16b >> mov v20.16b, v16.16b ==> neg v18.4s, v17.4s >> bsl v20.16b, v19.16b, v18.16b bsl v19.16b, v18.16b, v17.16b >> add v19.4s, v20.4s, v17.4s >> mov v18.16b, v16.16b >> bsl v18.16b, v19.16b, v20.16b >> >> and with SVE: >> >> mov z16.s, #-1 >> mov z17.s, #1==> neg z16.s, p0/m, z16.s >> eor z18.s, p0/m, z18.s, z16.s >> add z18.s, p0/m, z18.s, z17.s >> >> Here are the performance gains for benchmarks (see [1][2]) on ARM and x86 >> machines(note that the non-masked negation benchmarks do not have any >> improvement on X86 since no instructions are changed): >> >> NEON: >> BenchmarkGain >> Byte128Vector.NEG1.029 >> Byte128Vector.NEGMasked 1.757 >> Short128Vector.NEG 1.041 >> Short128Vector.NEGMasked 1.659 >> Int128Vector.NEG 1.005 >> Int128Vector.NEGMasked 1.513 >> Long128Vector.NEG1.003 >> Long128Vector.NEGMasked 1.878 >> >> SVE with 512-bits: >> BenchmarkGain >> ByteMaxVector.NEG1.10 >> ByteMaxVector.NEGMasked 1.165 >> ShortMaxVector.NEG 1.056 >> ShortMaxVector.NEGMasked 1.195 >> IntMaxVector.NEG 1.002 >> IntMaxVector.NEGMasked 1.239 >> LongMaxVector.NEG1.031 >> LongMaxVector.NEGMasked 1.191 >> >> X86 (non AVX-512): >> BenchmarkGain >> ByteMaxVector.NEGMasked 1.254 >> ShortMaxVector.NEGMasked 1.359 >> IntMaxVector.NEGMasked 1.431 >> LongMaxVector.NEGMasked 1.989 >> >> [1] >> https://github.com/openjdk/panama-vector/blob/vectorIntrinsics/test/micro/org/openjdk/bench/jdk/incubator/vector/operation/Byte128Vector.java#L1881 >> [2] >> https://github.com/openjdk/panama-vector/blob/vectorIntrinsics/test/micro/org/openjdk/bench/jdk/incubator/vector/operation/Byte128Vector.java#L1896 > > Xiaohong Gong has updated the pull request incrementally with one additional > commit since the last revision: > > Make "degenerate_vector_integral_negate" to be "NegVI" private Obvious performance improvement had ben observed on x86 for integral vector negation. So I think it's good to go. LGTM Thanks. Note: I didn't check the aarch64 code change. - Marked as reviewed by jiefu (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7782
Re: RFR: 8282162: [vector] Optimize vector negation API
On Tue, 15 Mar 2022 02:47:20 GMT, Xiaohong Gong wrote:
> > Note that in terms of Java semantics, negation of floating point values
> > needs to be implemented as subtraction from negative zero rather than
> > positive zero:
> > double negate(double arg) {return -0.0 - arg; }
> > This is to handle signed zeros correctly.
>
> Hi @jddarcy ,thanks for looking at this PR and thanks for the notes on the
> floating point negation! Yeah, this really makes sense to me. Kindly note
> that this patch didn't touch the negation of the floating point values. For
> Vector API, the vector floating point negation has been intrinsified to
> `NegVF/D` node by compiler that we directly generate the negation
> instructions for them. Thanks!
I would suggest changing the JBS title like `[vector] Optimize non-floating
vector negation API` .
-
PR: https://git.openjdk.java.net/jdk/pull/7782
Re: RFR: 8282162: [vector] Optimize vector negation API [v2]
On Tue, 22 Mar 2022 09:58:23 GMT, Xiaohong Gong wrote:
>> The current vector `"NEG"` is implemented with substraction a vector by zero
>> in case the architecture does not support the negation instruction. And to
>> fit the predicate feature for architectures that support it, the masked
>> vector `"NEG" ` is implemented with pattern `"v.not(m).add(1, m)"`. They
>> both can be optimized to a single negation instruction for ARM SVE.
>> And so does the non-masked "NEG" for NEON. Besides, implementing the masked
>> "NEG" with substraction for architectures that support neither negation
>> instruction nor predicate feature can also save several instructions than
>> the current pattern.
>>
>> To optimize the VectorAPI negation, this patch moves the implementation from
>> Java side to hotspot. The compiler will generate different nodes according
>> to the architecture:
>> - Generate the (predicated) negation node if architecture supports it,
>> otherwise, generate "`zero.sub(v)`" pattern for non-masked operation.
>> - Generate `"zero.sub(v, m)"` for masked operation if the architecture
>> does not have predicate feature, otherwise generate the original pattern
>> `"v.xor(-1, m).add(1, m)"`.
>>
>> So with this patch, the following transformations are applied:
>>
>> For non-masked negation with NEON:
>>
>> moviv16.4s, #0x0
>> sub v17.4s, v16.4s, v17.4s ==> neg v17.4s, v17.4s
>>
>> and with SVE:
>>
>> mov z16.s, #0
>> sub z18.s, z16.s, z17.s ==> neg z16.s, p7/m, z16.s
>>
>> For masked negation with NEON:
>>
>> moviv17.4s, #0x1
>> mvn v19.16b, v18.16b
>> mov v20.16b, v16.16b ==> neg v18.4s, v17.4s
>> bsl v20.16b, v19.16b, v18.16b bsl v19.16b, v18.16b, v17.16b
>> add v19.4s, v20.4s, v17.4s
>> mov v18.16b, v16.16b
>> bsl v18.16b, v19.16b, v20.16b
>>
>> and with SVE:
>>
>> mov z16.s, #-1
>> mov z17.s, #1==> neg z16.s, p0/m, z16.s
>> eor z18.s, p0/m, z18.s, z16.s
>> add z18.s, p0/m, z18.s, z17.s
>>
>> Here are the performance gains for benchmarks (see [1][2]) on ARM and x86
>> machines(note that the non-masked negation benchmarks do not have any
>> improvement on X86 since no instructions are changed):
>>
>> NEON:
>> BenchmarkGain
>> Byte128Vector.NEG1.029
>> Byte128Vector.NEGMasked 1.757
>> Short128Vector.NEG 1.041
>> Short128Vector.NEGMasked 1.659
>> Int128Vector.NEG 1.005
>> Int128Vector.NEGMasked 1.513
>> Long128Vector.NEG1.003
>> Long128Vector.NEGMasked 1.878
>>
>> SVE with 512-bits:
>> BenchmarkGain
>> ByteMaxVector.NEG1.10
>> ByteMaxVector.NEGMasked 1.165
>> ShortMaxVector.NEG 1.056
>> ShortMaxVector.NEGMasked 1.195
>> IntMaxVector.NEG 1.002
>> IntMaxVector.NEGMasked 1.239
>> LongMaxVector.NEG1.031
>> LongMaxVector.NEGMasked 1.191
>>
>> X86 (non AVX-512):
>> BenchmarkGain
>> ByteMaxVector.NEGMasked 1.254
>> ShortMaxVector.NEGMasked 1.359
>> IntMaxVector.NEGMasked 1.431
>> LongMaxVector.NEGMasked 1.989
>>
>> [1]
>> https://github.com/openjdk/panama-vector/blob/vectorIntrinsics/test/micro/org/openjdk/bench/jdk/incubator/vector/operation/Byte128Vector.java#L1881
>> [2]
>> https://github.com/openjdk/panama-vector/blob/vectorIntrinsics/test/micro/org/openjdk/bench/jdk/incubator/vector/operation/Byte128Vector.java#L1896
>
> Xiaohong Gong has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add a superclass for vector negation
src/hotspot/share/opto/vectornode.cpp line 1592:
> 1590:
> 1591: // Generate other vector nodes to implement the masked/non-masked
> vector negation.
> 1592: Node* VectorNode::degenerate_vector_integral_negate(Node* n, int vlen,
> BasicType bt, PhaseGVN* phase, bool is_predicated) {
Shall we move this declaration in `class NegVNode` since it is only used by
NegVNode::Ideal ?
-
PR: https://git.openjdk.java.net/jdk/pull/7782
Re: RFR: 8282162: [vector] Optimize vector negation API [v2]
On Mon, 21 Mar 2022 01:19:57 GMT, Xiaohong Gong wrote:
> The compiler can get the real type info from `Op_NegVI` that can also handle
> the `BYTE ` and `SHORT ` basic type. I just don't want to add more new IRs
> which also need more match rules in the ad files.
>
> > Is there any performance drop for byte/short negation operation if both of
> > them are handled as a NegVI vector?
>
> From the benchmark results I showed in the commit message, I didn't see not
> any performance drop for byte/short. Thanks!
There seems no vectorized negation instructions for {byte, short, int, long} on
x86, so this should be fine on x86.
I tested the patch on x86 and the performance number looks good.
-
PR: https://git.openjdk.java.net/jdk/pull/7782
Re: RFR: 8282162: [vector] Optimize vector negation API
On Sat, 19 Mar 2022 03:11:12 GMT, Jie Fu wrote:
>>> Note that in terms of Java semantics, negation of floating point values
>>> needs to be implemented as subtraction from negative zero rather than
>>> positive zero:
>>>
>>> double negate(double arg) {return -0.0 - arg; }
>>>
>>> This is to handle signed zeros correctly.
>>
>> Hi @jddarcy ,thanks for looking at this PR and thanks for the notes on the
>> floating point negation! Yeah, this really makes sense to me. Kindly note
>> that this patch didn't touch the negation of the floating point values. For
>> Vector API, the vector floating point negation has been intrinsified to
>> `NegVF/D` node by compiler that we directly generate the negation
>> instructions for them. Thanks!
>
>> Note that in terms of Java semantics, negation of floating point values
>> needs to be implemented as subtraction from negative zero rather than
>> positive zero:
>>
>> double negate(double arg) {return -0.0 - arg; }
>>
>> This is to handle signed zeros correctly.
>
> This seems easy to be broken by an opt enhancement.
> Just wondering do we have a jtreg test for this point? @jddarcy
> Thanks.
> Hi @DamonFool , thanks for your review! All the comments have been addressed.
> Thanks!
Thanks @XiaohongGong for the update.
And sorry for the late (just a little busy this week).
I'll do some testing and feedback here.
-
PR: https://git.openjdk.java.net/jdk/pull/7782
Re: RFR: 8283279: [Testbug] Improve TestGetSwapSpaceSize [v2]
On Wed, 23 Mar 2022 09:27:07 GMT, Severin Gehwolf wrote: >> Please review this container test improvement. The test in question only >> makes sense iff the total swap space size as reported by the container aware >> OperatingSystemMXBean is `0`. If that's not the case, then something else >> might be amiss, e.g. OperatingSystemMXBean reporting the host swap limits. >> In such a case a passing test tells us nothing. Certainly not if the >> fix from [JDK-8242480](https://bugs.openjdk.java.net/browse/JDK-8242480) is >> present or not. >> >> Testing: >> - [x] Manual with and without the code fix of JDK-8242480. Still passes with >> the fix, and fails without. Tested the test on cgroups v1 and cgroups v2. > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Incorporate review feedback > - Merge branch 'master' into test_getswapspace_improvement_8283279 > - 8283279: [Testbug] Improve TestGetSwapSpaceSize LGTM Thanks for the update. - Marked as reviewed by jiefu (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7854
Re: RFR: 8283279: [Testbug] Improve TestGetSwapSpaceSize
On Thu, 17 Mar 2022 13:40:53 GMT, Severin Gehwolf wrote:
> Please review this container test improvement. The test in question only
> makes sense iff the total swap space size as reported by the container aware
> OperatingSystemMXBean is `0`. If that's not the case, then something else
> might be amiss, e.g. OperatingSystemMXBean reporting the host swap limits. In
> such a case a passing test tells us nothing. Certainly not if the
> fix from [JDK-8242480](https://bugs.openjdk.java.net/browse/JDK-8242480) is
> present or not.
>
> Testing:
> - [x] Manual with and without the code fix of JDK-8242480. Still passes with
> the fix, and fails without. Tested the test on cgroups v1 and cgroups v2.
Please also update the copyright year.
Thanks.
test/jdk/jdk/internal/platform/docker/GetFreeSwapSpaceSize.java line 28:
> 26:
> 27: // Usage:
> 28: // GetFreeSwapSpaceSize
>
I would suggest
// GetFreeSwapSpaceSize
test/jdk/jdk/internal/platform/docker/GetFreeSwapSpaceSize.java line 32:
> 30: public static void main(String[] args) {
> 31: if (args.length != 4) {
> 32: throw new RuntimeException("Unexpected arguments. Expected 2,
> got " + args.length);
Shouldn't be `Expected 4` ?
-
PR: https://git.openjdk.java.net/jdk/pull/7854
Re: RFR: 8282162: [vector] Optimize vector negation API
On Tue, 15 Mar 2022 02:47:20 GMT, Xiaohong Gong wrote:
> Note that in terms of Java semantics, negation of floating point values needs
> to be implemented as subtraction from negative zero rather than positive zero:
>
> double negate(double arg) {return -0.0 - arg; }
>
> This is to handle signed zeros correctly.
This seems easy to be broken by an opt enhancement.
Just wondering do we have a jtreg test for this point? @jddarcy
Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/7782
Re: RFR: 8282162: [vector] Optimize vector negation API
On Fri, 11 Mar 2022 06:29:22 GMT, Xiaohong Gong wrote:
> The current vector `"NEG"` is implemented with substraction a vector by zero
> in case the architecture does not support the negation instruction. And to
> fit the predicate feature for architectures that support it, the masked
> vector `"NEG" ` is implemented with pattern `"v.not(m).add(1, m)"`. They both
> can be optimized to a single negation instruction for ARM SVE.
> And so does the non-masked "NEG" for NEON. Besides, implementing the masked
> "NEG" with substraction for architectures that support neither negation
> instruction nor predicate feature can also save several instructions than the
> current pattern.
>
> To optimize the VectorAPI negation, this patch moves the implementation from
> Java side to hotspot. The compiler will generate different nodes according to
> the architecture:
> - Generate the (predicated) negation node if architecture supports it,
> otherwise, generate "`zero.sub(v)`" pattern for non-masked operation.
> - Generate `"zero.sub(v, m)"` for masked operation if the architecture does
> not have predicate feature, otherwise generate the original pattern
> `"v.xor(-1, m).add(1, m)"`.
>
> So with this patch, the following transformations are applied:
>
> For non-masked negation with NEON:
>
> moviv16.4s, #0x0
> sub v17.4s, v16.4s, v17.4s ==> neg v17.4s, v17.4s
>
> and with SVE:
>
> mov z16.s, #0
> sub z18.s, z16.s, z17.s ==> neg z16.s, p7/m, z16.s
>
> For masked negation with NEON:
>
> moviv17.4s, #0x1
> mvn v19.16b, v18.16b
> mov v20.16b, v16.16b ==> neg v18.4s, v17.4s
> bsl v20.16b, v19.16b, v18.16b bsl v19.16b, v18.16b, v17.16b
> add v19.4s, v20.4s, v17.4s
> mov v18.16b, v16.16b
> bsl v18.16b, v19.16b, v20.16b
>
> and with SVE:
>
> mov z16.s, #-1
> mov z17.s, #1==> neg z16.s, p0/m, z16.s
> eor z18.s, p0/m, z18.s, z16.s
> add z18.s, p0/m, z18.s, z17.s
>
> Here are the performance gains for benchmarks (see [1][2]) on ARM and x86
> machines(note that the non-masked negation benchmarks do not have any
> improvement on X86 since no instructions are changed):
>
> NEON:
> BenchmarkGain
> Byte128Vector.NEG1.029
> Byte128Vector.NEGMasked 1.757
> Short128Vector.NEG 1.041
> Short128Vector.NEGMasked 1.659
> Int128Vector.NEG 1.005
> Int128Vector.NEGMasked 1.513
> Long128Vector.NEG1.003
> Long128Vector.NEGMasked 1.878
>
> SVE with 512-bits:
> BenchmarkGain
> ByteMaxVector.NEG1.10
> ByteMaxVector.NEGMasked 1.165
> ShortMaxVector.NEG 1.056
> ShortMaxVector.NEGMasked 1.195
> IntMaxVector.NEG 1.002
> IntMaxVector.NEGMasked 1.239
> LongMaxVector.NEG1.031
> LongMaxVector.NEGMasked 1.191
>
> X86 (non AVX-512):
> BenchmarkGain
> ByteMaxVector.NEGMasked 1.254
> ShortMaxVector.NEGMasked 1.359
> IntMaxVector.NEGMasked 1.431
> LongMaxVector.NEGMasked 1.989
>
> [1]
> https://github.com/openjdk/panama-vector/blob/vectorIntrinsics/test/micro/org/openjdk/bench/jdk/incubator/vector/operation/Byte128Vector.java#L1881
> [2]
> https://github.com/openjdk/panama-vector/blob/vectorIntrinsics/test/micro/org/openjdk/bench/jdk/incubator/vector/operation/Byte128Vector.java#L1896
src/hotspot/share/opto/vectorIntrinsics.cpp line 209:
> 207: #ifndef PRODUCT
> 208: if (C->print_intrinsics()) {
> 209: tty->print_cr(" ** Rejected vector op (%s,%s,%d) because
> architecture does not support variable vector negate",
"variable vector negate" seems a bit strange to me.
How about removing "variable"?
src/hotspot/share/opto/vectorIntrinsics.cpp line 291:
> 289: if ((mask_use_type & VecMaskUsePred) != 0) {
> 290: if (!Matcher::has_predicated_vectors()) {
> 291: return false;
If we return here, we would miss the intrinsic failing msg "Rejected vector
mask predicate using ...", right?
src/hotspot/share/opto/vectornode.cpp line 141:
> 139: case T_BYTE:
> 140: case T_SHORT:
> 141: case T_INT: return Op_NegVI;
Why not add `Op_NegVB` for `BYTE` and `Op_NegVS` for `SHORT`?
Is there any performance drop for byte/short negation operation if both of them
are handled as a NegVI vector?
src/hotspot/share/opto/vectornode.cpp line 1635:
> 1633: }
> 1634:
> 1635: Node* NegVINode::Ideal(PhaseGVN* phase, bool can_reshape) {
Much duplication in `NegVINode::Ideal` and `NegVLNode::Ideal`.
Is it possible to refactor the implementation?
-
PR: https://git.openjdk.java.net/jdk/pull/7782
Re: RFR: JDK-8278014: [vectorapi] Remove test run script [v2]
On Tue, 30 Nov 2021 23:31:21 GMT, Paul Sandoz wrote: >> Remove Vector API scripts for building and running tests. `jtreg` should be >> used instead. >> >> Also updated the test generation script to remove options that assume >> mercurial as the code repository. > > Paul Sandoz has updated the pull request incrementally with one additional > commit since the last revision: > > Update copywrite year. Thanks for your update. LGTM - Marked as reviewed by jiefu (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6621
Re: RFR: JDK-8278014: [vectorapi] Remove test run script
On Tue, 30 Nov 2021 19:22:53 GMT, Paul Sandoz wrote: > Remove Vector API scripts for building and running tests. `jtreg` should be > used instead. > > Also updated the test generation script to remove options that assume > mercurial as the code repository. Shall we also update the copyright year? - PR: https://git.openjdk.java.net/jdk/pull/6621
Re: RFR: 8273169: java/util/regex/NegativeArraySize.java failed after JDK-8271302
On Tue, 31 Aug 2021 09:40:14 GMT, xpbob wrote: > 8273169: java/util/regex/NegativeArraySize.java failed after JDK-8271302 LGTM Thanks for fixing it. - Marked as reviewed by jiefu (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5315
Re: [jdk17] RFR: 8067223: [TESTBUG] Rename Whitebox API package [v2]
On Sat, 31 Jul 2021 20:42:10 GMT, Igor Ignatyev wrote: >> Hi all, >> >> could you please review this big tedious and trivial(-ish) patch which moves >> `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package? >> >> the majority of the patch is the following substitutions: >> - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g` >> - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g` >> - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g` >> - `s/sun.hotspot.code/jdk.test.whitebox.code/g` >> - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g` >> - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g` >> >> testing: tier1-4 >> >> Thanks, >> -- Igor > > Igor Ignatyev has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains 12 new > commits since the last revision: > > - fixed ctw build > - updated runtime/cds/appcds/JarBuilder to copy j.t.w.WhiteBox's inner class > - updated requires.VMProps > - updated TEST.ROOT > - adjusted hotspot source > - added test > - moved and adjusted WhiteBox tests (test/lib-test/sun/hotspot/whitebox) > - updated ClassFileInstaller to copy j.t.w.WhiteBox's inner class > - removed sun/hotspot/parser/DiagnosticCommand > - deprecated sun/hotspot classes >disallowed s.h.WhiteBox w/ security manager > - ... and 2 more: > https://git.openjdk.java.net/jdk17/compare/8f12f2cf...237e8860 Shall we also update the copyright year like test/lib/sun/hotspot/cpuinfo/CPUInfo.java? Thanks. - PR: https://git.openjdk.java.net/jdk17/pull/290
Re: [jdk17] RFR: 8268353: Test libsvml.so is and is not present in jdk image [v2]
On Wed, 16 Jun 2021 15:29:19 GMT, Paul Sandoz wrote: >> Test that when the jdk.incubator.vector module is present that libsvml.so is >> present, and test the opposite case. > > Paul Sandoz has updated the pull request incrementally with one additional > commit since the last revision: > > Require C2. LGTM Thanks for your update. - Marked as reviewed by jiefu (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/47
Re: [jdk17] RFR: 8268353: Test libsvml.so is and is not present in jdk image
On Mon, 14 Jun 2021 16:06:04 GMT, Paul Sandoz wrote: > Test that when the jdk.incubator.vector module is present that libsvml.so is > present, and test the opposite case. The test logic should be changed. If C2 is absent, libsvml.so would not be generated after JDK-8268643. Thanks. - Changes requested by jiefu (Reviewer). PR: https://git.openjdk.java.net/jdk17/pull/47
Re: RFR: 8267357: build breaks with -Werror option on micro benchmark added for JDK-8256973
On Wed, 19 May 2021 08:20:13 GMT, Jatin Bhateja wrote: > Relevant declarations modified and tested with -Werror, no longer see > unchecked conversion warnings. > > Kindly review and approve. LGTM - Marked as reviewed by jiefu (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4108
Integrated: 8264809: test-lib fails to build due to some warnings in ASN1Formatter and jfr
On Wed, 7 Apr 2021 03:34:15 GMT, Jie Fu wrote: > Hi all, > > test-lib fails to build due to three javac warnings: This pull request has now been integrated. Changeset: 88eb2919 Author: Jie Fu URL: https://git.openjdk.java.net/jdk/commit/88eb2919 Stats: 5 lines in 3 files changed: 1 ins; 0 del; 4 mod 8264809: test-lib fails to build due to some warnings in ASN1Formatter and jfr Reviewed-by: rriggs - PR: https://git.openjdk.java.net/jdk/pull/3367
Re: RFR: 8264809: test-lib fails to build due to some warnings in ASN1Formatter and jfr
On Wed, 7 Apr 2021 13:48:57 GMT, Roger Riggs wrote: >> Hi all, >> >> test-lib fails to build due to three javac warnings: > > Looks good. > It seems only the build-test-lib make rule has error on warnings set. > When running the tests, the compiles do not fail. (though the warnings are > produced). Thanks @RogerRiggs . - PR: https://git.openjdk.java.net/jdk/pull/3367
RFR: 8264809: test-lib fails to build due to some warnings in ASN1Formatter and jfr
Hi all, test-lib fails to build due to three javac warnings: - Commit messages: - 8264809: test-lib fails to build due to some warnings in ASN1Formatter and jfr Changes: https://git.openjdk.java.net/jdk/pull/3367/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=3367=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264809 Stats: 5 lines in 3 files changed: 1 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/3367.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3367/head:pull/3367 PR: https://git.openjdk.java.net/jdk/pull/3367
Withdrawn: 8259925: [Vector API] Unreasonable IndexOutOfBoundsException message when length < vlen
On Mon, 18 Jan 2021 13:32:24 GMT, Jie Fu wrote:
> Hi all,
>
> For this reproducer:
>
> import jdk.incubator.vector.ByteVector;
> import jdk.incubator.vector.VectorSpecies;
>
> public class Test {
> static final VectorSpecies SPECIES_128 = ByteVector.SPECIES_128;
> static byte[] a = new byte[8];
> static byte[] b = new byte[8];
>
> public static void main(String[] args) {
> ByteVector av = ByteVector.fromArray(SPECIES_128, a, 0);
> av.intoArray(b, 0);
> System.out.println("b: " + b[0]);
> }
> }
>
> The following IndexOutOfBoundsException message (length -7) is unreasonable.
>
> Exception in thread "main" java.lang.IndexOutOfBoundsException: Index 0 out
> of bounds for length -7
>
> It might be better to fix it like this.
>
> Exception in thread "main" java.lang.IndexOutOfBoundsException: Index 0 out
> of bounds for length 0
>
> Thanks.
> Best regards,
> Jie
This pull request has been closed without being integrated.
-
PR: https://git.openjdk.java.net/jdk/pull/2127
Integrated: 8262096: Vector API fails to work due to VectorShape initialization exception
On Thu, 25 Feb 2021 09:31:01 GMT, Jie Fu wrote:
> Hi all,
>
> Vector API fails to work when:
> - case 1: MaxVectorSize is set to <=8, or
> - case 2: C2 is disabled
>
> The reason is that {max/preferred} VectorShape initialization fails in both
> cases.
> And the root cause is that VectorSupport_GetMaxLaneCount [1] returns
> unreasonable values (0 for case 1 and -1 for case 2).
>
> Vector API should not depend on C2 to run.
> It should work even there is no JIT compiler since it's a Java-level api.
> So let's fix it.
>
> Testing:
> - jdk/incubator/vector with -XX:MaxVectorSize=default/8 on Linux/x64
>
> Thanks.
> Best regards,
> Jie
>
> [1]
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/vectorSupport.cpp#L364
This pull request has now been integrated.
Changeset: 40bdf52e
Author:Jie Fu
URL: https://git.openjdk.java.net/jdk/commit/40bdf52e
Stats: 60 lines in 2 files changed: 38 ins; 10 del; 12 mod
8262096: Vector API fails to work due to VectorShape initialization exception
Reviewed-by: psandoz, vlivanov
-
PR: https://git.openjdk.java.net/jdk/pull/2722
Re: RFR: 8262096: Vector API fails to work due to VectorShape initialization exception [v7]
On Tue, 2 Mar 2021 17:17:37 GMT, Paul Sandoz wrote: >> Jie Fu has updated the pull request incrementally with one additional commit >> since the last revision: >> >> Add VectorShape.getMaxVectorBitSize > > Marked as reviewed by psandoz (Reviewer). > The `PreferredSpeciesTest` also needs to be executed with no HotSpot flags. Yes, we already have this one [1]. So no need to be updated and will push it later. Thanks. [1] https://github.com/openjdk/jdk/blob/master/test/jdk/jdk/incubator/vector/PreferredSpeciesTest.java#L33 - PR: https://git.openjdk.java.net/jdk/pull/2722
Re: RFR: 8262096: Vector API fails to work due to VectorShape initialization exception [v5]
On Sun, 28 Feb 2021 13:31:38 GMT, Jie Fu wrote:
>> `@requires vm.compiler2.enabled` had been added.
>> Thanks.
>
> @PaulSandoz , are you also OK with the latest version?
> Thanks.
> @DamonFool I think Vladimir is correct in the layering, in this respect i
> think we can make things a littler clearer. This seems like a small thing but
> i think its worth making very explicit as there is some hidden complexity.
>
> What if we add the following method to `VectorShape`:
>
> ```java
> /**
> * Returns the maximum vector bit size for a given element type.
> *
> * @param etype the element type.
> * @return the maximum vector bit.
> */
> /*package-private*/
> static int getMaxVectorBitSize(Class etype) {
> // May return -1 if C2 is not enabled,
> // or a value smaller than the S_64_BIT.vectorBitSize /
> elementSizeInBits, on say 32-bit platforms
> // If so default to S_64_BIT
> int maxLaneCount = VectorSupport.getMaxLaneCount(etype);
> int elementSizeInBits = LaneType.of(etype).elementSize;
> return Math.max(maxLaneCount * elementSizeInBits,
> S_64_BIT.vectorBitSize);
> }
> ```
>
> It is package private so it can be tested explicitly if need be.
>
> Then we can reuse that method:
>
> ```
> S_Max_BIT(getMaxVectorBitSize(byte.class));
> ```
>
> ```
> static VectorShape largestShapeFor(Class etype) {
> return VectorShape.forBitSize(getMaxVectorBitSize(etype));
> }
> ```
>
> I think that's correct, but i have not tested. WDYT?
Good suggestion.
Updated.
Testing:
- jdk/incubator/vector with MaxVectorSize=default/8/4 on Linux/x64
- jdk/incubator/vector without C2 on Linux/x64
Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/2722
Re: RFR: 8262096: Vector API fails to work due to VectorShape initialization exception [v7]
> Hi all,
>
> Vector API fails to work when:
> - case 1: MaxVectorSize is set to <=8, or
> - case 2: C2 is disabled
>
> The reason is that {max/preferred} VectorShape initialization fails in both
> cases.
> And the root cause is that VectorSupport_GetMaxLaneCount [1] returns
> unreasonable values (0 for case 1 and -1 for case 2).
>
> Vector API should not depend on C2 to run.
> It should work even there is no JIT compiler since it's a Java-level api.
> So let's fix it.
>
> Testing:
> - jdk/incubator/vector with -XX:MaxVectorSize=default/8 on Linux/x64
>
> Thanks.
> Best regards,
> Jie
>
> [1]
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/vectorSupport.cpp#L364
Jie Fu has updated the pull request incrementally with one additional commit
since the last revision:
Add VectorShape.getMaxVectorBitSize
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/2722/files
- new: https://git.openjdk.java.net/jdk/pull/2722/files/79402411..d3b4cc35
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=2722=06
- incr: https://webrevs.openjdk.java.net/?repo=jdk=2722=05-06
Stats: 26 lines in 1 file changed: 16 ins; 6 del; 4 mod
Patch: https://git.openjdk.java.net/jdk/pull/2722.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2722/head:pull/2722
PR: https://git.openjdk.java.net/jdk/pull/2722
Re: RFR: 8262096: Vector API fails to work due to VectorShape initialization exception [v5]
On Sat, 27 Feb 2021 11:15:06 GMT, Jie Fu wrote: >> test/jdk/jdk/incubator/vector/PreferredSpeciesTest.java line 42: >> >>> 40: * @modules jdk.incubator.vector java.base/jdk.internal.vm.vector >>> 41: * @run testng/othervm -XX:MaxVectorSize=8 PreferredSpeciesTest >>> 42: * @run testng/othervm -XX:MaxVectorSize=4 PreferredSpeciesTest >> >> `-XX:MaxVectorSize` is C2-specific. It's better to specify either >> `-XX:-IgnoreUnrecognizedVMOptions` or `@requires vm.compiler2.enabled`. > > `@requires vm.compiler2.enabled` had been added. > Thanks. @PaulSandoz , are you also OK with the latest version? Thanks. - PR: https://git.openjdk.java.net/jdk/pull/2722
Re: RFR: 8262096: Vector API fails to work due to VectorShape initialization exception [v6]
> Hi all,
>
> Vector API fails to work when:
> - case 1: MaxVectorSize is set to <=8, or
> - case 2: C2 is disabled
>
> The reason is that {max/preferred} VectorShape initialization fails in both
> cases.
> And the root cause is that VectorSupport_GetMaxLaneCount [1] returns
> unreasonable values (0 for case 1 and -1 for case 2).
>
> Vector API should not depend on C2 to run.
> It should work even there is no JIT compiler since it's a Java-level api.
> So let's fix it.
>
> Testing:
> - jdk/incubator/vector with -XX:MaxVectorSize=default/8 on Linux/x64
>
> Thanks.
> Best regards,
> Jie
>
> [1]
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/vectorSupport.cpp#L364
Jie Fu has updated the pull request incrementally with one additional commit
since the last revision:
Add requires vm.compiler2.enabled
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/2722/files
- new: https://git.openjdk.java.net/jdk/pull/2722/files/b67b232d..79402411
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=2722=05
- incr: https://webrevs.openjdk.java.net/?repo=jdk=2722=04-05
Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod
Patch: https://git.openjdk.java.net/jdk/pull/2722.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2722/head:pull/2722
PR: https://git.openjdk.java.net/jdk/pull/2722
Re: RFR: 8262096: Vector API fails to work due to VectorShape initialization exception [v5]
On Sat, 27 Feb 2021 10:58:16 GMT, Vladimir Ivanov wrote: >> Jie Fu has updated the pull request with a new target base due to a merge or >> a rebase. The incremental webrev excludes the unrelated changes brought in >> by the merge/rebase. The pull request contains seven additional commits >> since the last revision: >> >> - Fix in jdk/incubator/vector/VectorShape.java >> - Merge branch 'master' into JDK-8262096 >> - Revert changes >> - Remove -XX:TieredStopAtLevel=3 >> - Update the jtreg test >> - The numerator should be 8 (byte) >> - 8262096: Vector API fails to work due to VectorShape initialization >> exception > > test/jdk/jdk/incubator/vector/PreferredSpeciesTest.java line 42: > >> 40: * @modules jdk.incubator.vector java.base/jdk.internal.vm.vector >> 41: * @run testng/othervm -XX:MaxVectorSize=8 PreferredSpeciesTest >> 42: * @run testng/othervm -XX:MaxVectorSize=4 PreferredSpeciesTest > > `-XX:MaxVectorSize` is C2-specific. It's better to specify either > `-XX:-IgnoreUnrecognizedVMOptions` or `@requires vm.compiler2.enabled`. `@requires vm.compiler2.enabled` had been added. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/2722
Re: RFR: 8262096: Vector API fails to work due to VectorShape initialization exception [v4]
On Fri, 26 Feb 2021 15:48:18 GMT, Vladimir Ivanov wrote:
> > I'd like to keep DoubleVector.SPECIES_PREFERRED.length() <=
> > VectorSupport.getMaxLaneCount(double.class) for Java programmers since the
> > VectorSupport_GetMaxLaneCount is used to implement a Java API.
>
> It doesn't make much sense to me. `VectorSupport` is an internal API for
> `jdk.incubator.vector` to consume.
> It's `jdk.incubator.vector` job to interpret the result and adapt accordingly.
Okay, I'm fine to fix it in jdk/incubator/vector/VectorShape.java if we don't
keep something like that.
For the updated fix, the {max/preferred} shape will be initialized as
shape-64-bit if hotspot doesn't support vectorization.
Testing:
- jdk/incubator/vector with MaxVectorSize=default/8/4 on Linux/x64
- jdk/incubator/vector without C2 on Linux/x64
Any comments?
Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/2722
Re: RFR: 8262096: Vector API fails to work due to VectorShape initialization exception [v5]
> Hi all,
>
> Vector API fails to work when:
> - case 1: MaxVectorSize is set to <=8, or
> - case 2: C2 is disabled
>
> The reason is that {max/preferred} VectorShape initialization fails in both
> cases.
> And the root cause is that VectorSupport_GetMaxLaneCount [1] returns
> unreasonable values (0 for case 1 and -1 for case 2).
>
> Vector API should not depend on C2 to run.
> It should work even there is no JIT compiler since it's a Java-level api.
> So let's fix it.
>
> Testing:
> - jdk/incubator/vector with -XX:MaxVectorSize=default/8 on Linux/x64
>
> Thanks.
> Best regards,
> Jie
>
> [1]
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/vectorSupport.cpp#L364
Jie Fu has updated the pull request with a new target base due to a merge or a
rebase. The incremental webrev excludes the unrelated changes brought in by the
merge/rebase. The pull request contains seven additional commits since the last
revision:
- Fix in jdk/incubator/vector/VectorShape.java
- Merge branch 'master' into JDK-8262096
- Revert changes
- Remove -XX:TieredStopAtLevel=3
- Update the jtreg test
- The numerator should be 8 (byte)
- 8262096: Vector API fails to work due to VectorShape initialization exception
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/2722/files
- new: https://git.openjdk.java.net/jdk/pull/2722/files/bbe6150c..b67b232d
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=2722=04
- incr: https://webrevs.openjdk.java.net/?repo=jdk=2722=03-04
Stats: 7047 lines in 380 files changed: 3707 ins; 1675 del; 1665 mod
Patch: https://git.openjdk.java.net/jdk/pull/2722.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2722/head:pull/2722
PR: https://git.openjdk.java.net/jdk/pull/2722
Re: RFR: 8262096: Vector API fails to work due to VectorShape initialization exception [v4]
On Fri, 26 Feb 2021 13:55:15 GMT, Vladimir Ivanov wrote: > IMO the fix should be in > `src/jdk.incubator.vector/share/classes/jdk/incubator/vector/VectorShape.java`. > > JVM does the right job when it signals vector support is absent (by returning > `-1`). > > `jdk.incubator.vector` implementation should take that into account and > choose a preferred shape for pure Java execution mode. Hi @iwanowww , Thanks for your review. >From the view of C2 compiler, you are right. But the Java programmer may be confused if we got something like DoubleVector.SPECIES_PREFERRED.length() > VectorSupport.getMaxLaneCount(double.class). I'd like to keep DoubleVector.SPECIES_PREFERRED.length() <= VectorSupport.getMaxLaneCount(double.class) for Java programmers since the VectorSupport_GetMaxLaneCount is used to implement a Java API. What do you think? Thanks. - PR: https://git.openjdk.java.net/jdk/pull/2722
Re: RFR: 8262096: Vector API fails to work due to VectorShape initialization exception [v4]
> Hi all,
>
> Vector API fails to work when:
> - case 1: MaxVectorSize is set to <=8, or
> - case 2: C2 is disabled
>
> The reason is that {max/preferred} VectorShape initialization fails in both
> cases.
> And the root cause is that VectorSupport_GetMaxLaneCount [1] returns
> unreasonable values (0 for case 1 and -1 for case 2).
>
> Vector API should not depend on C2 to run.
> It should work even there is no JIT compiler since it's a Java-level api.
> So let's fix it.
>
> Testing:
> - jdk/incubator/vector with -XX:MaxVectorSize=default/8 on Linux/x64
>
> Thanks.
> Best regards,
> Jie
>
> [1]
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/vectorSupport.cpp#L364
Jie Fu has updated the pull request incrementally with one additional commit
since the last revision:
Remove -XX:TieredStopAtLevel=3
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/2722/files
- new: https://git.openjdk.java.net/jdk/pull/2722/files/aa475b0a..bbe6150c
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=2722=03
- incr: https://webrevs.openjdk.java.net/?repo=jdk=2722=02-03
Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod
Patch: https://git.openjdk.java.net/jdk/pull/2722.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2722/head:pull/2722
PR: https://git.openjdk.java.net/jdk/pull/2722
Re: RFR: 8262096: Vector API fails to work due to VectorShape initialization exception [v4]
On Fri, 26 Feb 2021 02:28:55 GMT, Paul Sandoz wrote: > In that case I think we can remove the execution with > `-XX:TieredStopAtLevel=x`. Fixed. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/2722
Re: RFR: 8262096: Vector API fails to work due to VectorShape initialization exception [v3]
> Hi all,
>
> Vector API fails to work when:
> - case 1: MaxVectorSize is set to <=8, or
> - case 2: C2 is disabled
>
> The reason is that {max/preferred} VectorShape initialization fails in both
> cases.
> And the root cause is that VectorSupport_GetMaxLaneCount [1] returns
> unreasonable values (0 for case 1 and -1 for case 2).
>
> Vector API should not depend on C2 to run.
> It should work even there is no JIT compiler since it's a Java-level api.
> So let's fix it.
>
> Testing:
> - jdk/incubator/vector with -XX:MaxVectorSize=default/8 on Linux/x64
>
> Thanks.
> Best regards,
> Jie
>
> [1]
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/vectorSupport.cpp#L364
Jie Fu has updated the pull request incrementally with one additional commit
since the last revision:
Update the jtreg test
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/2722/files
- new: https://git.openjdk.java.net/jdk/pull/2722/files/724b36d4..aa475b0a
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=2722=02
- incr: https://webrevs.openjdk.java.net/?repo=jdk=2722=01-02
Stats: 59 lines in 2 files changed: 10 ins; 48 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/2722.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2722/head:pull/2722
PR: https://git.openjdk.java.net/jdk/pull/2722
Re: RFR: 8262096: Vector API fails to work due to VectorShape initialization exception [v3]
On Fri, 26 Feb 2021 00:30:45 GMT, Paul Sandoz wrote: > Thanks, was the test `VectorShapeInitTest` passing prior to the fix of the > numerator? Yes. It also passed with min_lane_count = 64 / type2aelembytes(bt). > Perhaps we should be testing more directly on > `VectorShape.S_Max_BIT.vectorBitSize()` and `VectorShape.preferredShape` ? Ok. But it that case, I'd like to just re-use jdk/incubator/vector/PreferredSpeciesTest.java. > Also, perhaps we can run with C2 disabled, with `-XX:TieredStopAtLevel=3` ? Fine. Although this bug wouldn't be triggered with -XX:TieredStopAtLevel=x, it can help test with C1. Patch had been updated. Any comments? Thanks. - PR: https://git.openjdk.java.net/jdk/pull/2722
Re: RFR: 8262096: Vector API fails to work due to VectorShape initialization exception [v2]
On Thu, 25 Feb 2021 17:23:11 GMT, Paul Sandoz wrote:
>> Jie Fu has updated the pull request incrementally with one additional commit
>> since the last revision:
>>
>> The numerator should be 8 (byte)
>
> src/hotspot/share/prims/vectorSupport.cpp line 368:
>
>> 366: if (java_lang_Class::is_primitive(mirror)) {
>> 367: BasicType bt = java_lang_Class::primitive_type(mirror);
>> 368: int min_lane_count = 64 / type2aelembytes(bt);
>
> I am uncertain of the units here. Is the numerator in bits and the
> denominator in bytes?
Thanks Paul for your review.
Oops, the numerator should be 8 (bytes).
Updated.
Testing of jdk/incubator/vector (MaxVector=default/8/4 or without C2) is still
fine.
Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/2722
Re: RFR: 8262096: Vector API fails to work due to VectorShape initialization exception [v2]
> Hi all,
>
> Vector API fails to work when:
> - case 1: MaxVectorSize is set to <=8, or
> - case 2: C2 is disabled
>
> The reason is that {max/preferred} VectorShape initialization fails in both
> cases.
> And the root cause is that VectorSupport_GetMaxLaneCount [1] returns
> unreasonable values (0 for case 1 and -1 for case 2).
>
> Vector API should not depend on C2 to run.
> It should work even there is no JIT compiler since it's a Java-level api.
> So let's fix it.
>
> Testing:
> - jdk/incubator/vector with -XX:MaxVectorSize=default/8 on Linux/x64
>
> Thanks.
> Best regards,
> Jie
>
> [1]
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/prims/vectorSupport.cpp#L364
Jie Fu has updated the pull request incrementally with one additional commit
since the last revision:
The numerator should be 8 (byte)
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/2722/files
- new: https://git.openjdk.java.net/jdk/pull/2722/files/09b49f25..724b36d4
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=2722=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=2722=00-01
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/2722.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2722/head:pull/2722
PR: https://git.openjdk.java.net/jdk/pull/2722
Re: RFR: 8261938: ASN1Formatter.annotate should not return in the finally block [v3]
On Fri, 19 Feb 2021 15:15:05 GMT, Roger Riggs wrote: >> Jie Fu has updated the pull request incrementally with one additional commit >> since the last revision: >> >> Catch exception and return the accumulated string > > Catching and ignoring the exception has the same behavior as handling it with > finally. Thanks @RogerRiggs . - PR: https://git.openjdk.java.net/jdk/pull/2620
Integrated: 8261938: ASN1Formatter.annotate should not return in the finally block
On Thu, 18 Feb 2021 02:28:01 GMT, Jie Fu wrote: > Hi all, > > ASN1Formatter.annotate should be able to throw an IOException according to > this comment [1]. > But it fails to do so due to the return [2] in the finally block, which would > swallow the IOException. > > Generally, it seems not good to put a return statement in a finally block > because it would overwrite other return-statements or Exceptions [3]. > > Thanks. > Best regards, > Jie > > [1] > https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L113 > [2] > https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L120 > [3] > https://stackoverflow.com/questions/17481251/finally-block-does-not-complete-normally-eclipse-warning This pull request has now been integrated. Changeset: b10376ba Author:Jie Fu URL: https://git.openjdk.java.net/jdk/commit/b10376ba Stats: 15 lines in 1 file changed: 10 ins; 1 del; 4 mod 8261938: ASN1Formatter.annotate should not return in the finally block Reviewed-by: rriggs - PR: https://git.openjdk.java.net/jdk/pull/2620
Re: RFR: 8261938: ASN1Formatter.annotate should not return in the finally block [v2]
On Fri, 19 Feb 2021 01:56:19 GMT, Roger Riggs wrote: > The formattters are a test component used both standalone and in the context > of the HexPrinter test utilities. > In typical use, the stream is a wrapped byte array, so there are no > exceptions other than EOF. > The choice of DataInputStream was chosen for the convenience of the methods > to read different types > and (declared) exceptions are an unwelcome artifact. > > Formatters are designed to be nested, where one formatter can call another > and the valuable output > is the formatted string that has been accumulated from the beginning of the > stream. > If an exception was percolated up and the formatted output discarded it would > defeat the purpose. > > If an exception was thrown, it would still return useful information about > the stream to that point. > The documentation could be improved to be clear on that point. Got it. Thanks for your clarification. Updated. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/2620
Re: RFR: 8261938: ASN1Formatter.annotate should not return in the finally block [v3]
> Hi all, > > ASN1Formatter.annotate should be able to throw an IOException according to > this comment [1]. > But it fails to do so due to the return [2] in the finally block, which would > swallow the IOException. > > Generally, it seems not good to put a return statement in a finally block > because it would overwrite other return-statements or Exceptions [3]. > > Thanks. > Best regards, > Jie > > [1] > https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L113 > [2] > https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L120 > [3] > https://stackoverflow.com/questions/17481251/finally-block-does-not-complete-normally-eclipse-warning Jie Fu has updated the pull request incrementally with one additional commit since the last revision: Catch exception and return the accumulated string - Changes: - all: https://git.openjdk.java.net/jdk/pull/2620/files - new: https://git.openjdk.java.net/jdk/pull/2620/files/b537e060..a1f29f33 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2620=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2620=01-02 Stats: 16 lines in 1 file changed: 13 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2620.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2620/head:pull/2620 PR: https://git.openjdk.java.net/jdk/pull/2620
Re: RFR: 8261938: ASN1Formatter.annotate should not return in the finally block [v2]
On Thu, 18 Feb 2021 14:53:17 GMT, Roger Riggs wrote: >> Jie Fu has updated the pull request incrementally with one additional commit >> since the last revision: >> >> Update the copyright year > > An EOFException can occur during the call to annotate() and must return the > accumulated contents of the StringBuffer. Otherwise it is discarded. Thanks @RogerRiggs for your review. Just want to make sure: 1. AFAIK, for a method, it seems impossible to return a value and throw an exception at the same time. Did you mean we just need to return a string with the IOException to be catched? 2. Does it make sense to return a string when an IOException happens? Thanks. - PR: https://git.openjdk.java.net/jdk/pull/2620
Re: RFR: 8261938: ASN1Formatter.annotate should not return in the finally block [v2]
> Hi all, > > ASN1Formatter.annotate should be able to throw an IOException according to > this comment [1]. > But it fails to do so due to the return [2] in the finally block, which would > swallow the IOException. > > Generally, it seems not good to put a return statement in a finally block > because it would overwrite other return-statements or Exceptions [3]. > > Thanks. > Best regards, > Jie > > [1] > https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L113 > [2] > https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L120 > [3] > https://stackoverflow.com/questions/17481251/finally-block-does-not-complete-normally-eclipse-warning Jie Fu has updated the pull request incrementally with one additional commit since the last revision: Update the copyright year - Changes: - all: https://git.openjdk.java.net/jdk/pull/2620/files - new: https://git.openjdk.java.net/jdk/pull/2620/files/c0af12b1..b537e060 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2620=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2620=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/2620.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2620/head:pull/2620 PR: https://git.openjdk.java.net/jdk/pull/2620
RFR: 8261938: ASN1Formatter.annotate should not return in the finally block
Hi all, ASN1Formatter.annotate should be able to throw an IOException according to this comment [1]. But it fails to do so due to the return [2] in the finally block, which would swallow the IOException. Generally, it seems not good to put a return statement in a finally block because it would overwrite other return-statements or Exceptions [3]. Thanks. Best regards, Jie [1] https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L113 [2] https://github.com/openjdk/jdk/blob/master/test/lib/jdk/test/lib/hexdump/ASN1Formatter.java#L120 [3] https://stackoverflow.com/questions/17481251/finally-block-does-not-complete-normally-eclipse-warning - Commit messages: - 8261938: ASN1Formatter.annotate should not return in the finally block Changes: https://git.openjdk.java.net/jdk/pull/2620/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2620=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261938 Stats: 5 lines in 1 file changed: 0 ins; 3 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/2620.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2620/head:pull/2620 PR: https://git.openjdk.java.net/jdk/pull/2620
Re: RFR: 8259925: [Vector API] Unreasonable IndexOutOfBoundsException message when length < vlen
On Tue, 26 Jan 2021 18:20:00 GMT, Paul Sandoz wrote: > Hi Jie, Thanks for the detailed analysis. I suspect the difference outside of > loops is because of expression "length - (vlen - 1)”. We need to make > intrinsic Objects.checkFromIndexSize. Is that something you might be > interested in pursuing? Yes, I'd like to do that in the future. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/2127
Re: RFR: 8259925: [Vector API] Unreasonable IndexOutOfBoundsException message when length < vlen
On Thu, 21 Jan 2021 16:54:36 GMT, Paul Sandoz wrote: > The intrinsic enables C2 to more reliably elide bounds checks. I don't know > the exact details but at a high-level it transforms signed values into > unsigned values (and therefore the signed comparisons become unsigned > comparisons), which helps C2 remove checks when there is a dominating check > of, for example, an upper loop bound. > > You say "the intrinsified Objects.checkIndex seems to generate more code than > inlined if-statements". Can you present some examples of Java code and the > corresponding C2 generated assembler where this happens? Hi @PaulSandoz , I agree with you that let's keep the code as it is for the sake of performance. I spent some time looking into the assembly code generated by Objects.checkIndex and inlined if-statements. Here are the test program [1], running script [2] and diff [3]. - For testSimple [4] that I checked last week, inlined if-statements [5] is better than Objects.checkIndex [6]. - However, for testLoop [7], Objects.checkIndex [8] is better than inlined if-statements [9]. (I'm sorry I didn't check loop methods last week.) AFAICS, the inlined if-statements will generate two more instructions [10] to check wether idx >= 0 for each loop iteration. It seems that the intrinsified Objects.checkIndex will be able to optimize out the lower bound check for loops. So I also agree with you that an intrinsified method seems the right choice. Thanks. Best regards, Jie [1] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/Bar.java [2] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/1.sh [3] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/1.diff [4] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/Bar.java#L10 [5] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/3-testSimple.log [6] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/2-testSimple.log [7] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/Bar.java#L15 [8] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/2-testLoop.log [9] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/3-testLoop.log [10] https://github.com/DamonFool/experiment/blob/main/JDK-8259925/3-testLoop.log#L135 - PR: https://git.openjdk.java.net/jdk/pull/2127
Re: RFR: 8259925: [Vector API] Unreasonable IndexOutOfBoundsException message when length < vlen
On Thu, 21 Jan 2021 16:54:36 GMT, Paul Sandoz wrote: >>> Unfortunately it is still problematic because you have replaced the >>> intrinsic check (that performed by `Object.checksIndex`, or more >>> specifically >>> [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/util/Preconditions.java#L261)). >>> >>> Further, you have replaced the bounds check options, in place for >>> experimentation. We are not ready yet to collapse our options, further >>> analysis is required as bounds checks can be very sensitive in C2. >>> >>> I would be open to you adding a third case, so that you can analyze the >>> performance without perturbing the default behavior. I suspect the correct >>> fix is to make intrinsic `Objects.checkFromIndexSize` in a similar manner >>> to `Object.checksIndex`. >> >> Hi @PaulSandoz , >> >> Thanks for your kind review and comments. >> >> To be honest, I didn't get your first point. >> As for the example above, the intrinsified Objects.checkIndex seems to >> generate more code than inlined if-statements. >> So I'm confused why it's a problem to replace the intrinsic check. >> Did you mean an intrinsic is always (or for most cases) better than >> non-intrinc or inlined if-statements? >> And why? >> >> Could you please make it more clearer to us? >> Thanks. > > The intrinsic enables C2 to more reliably elide bounds checks. I don't know > the exact details but at a high-level it transforms signed values into > unsigned values (and therefore the signed comparisons become unsigned > comparisons), which helps C2 remove checks when there is a dominating check > of, for example, an upper loop bound. > > You say "the intrinsified Objects.checkIndex seems to generate more code than > inlined if-statements". Can you present some examples of Java code and the > corresponding C2 generated assembler where this happens? Hi @PaulSandoz , I will show you the assembly code next week since it is already Friday night now. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/2127
Re: RFR: 8259925: [Vector API] Unreasonable IndexOutOfBoundsException message when length < vlen
On Thu, 21 Jan 2021 16:54:36 GMT, Paul Sandoz wrote: > The intrinsic enables C2 to more reliably elide bounds checks. I don't know > the exact details but at a high-level it transforms signed values into > unsigned values (and therefore the signed comparisons become unsigned > comparisons), which helps C2 remove checks when there is a dominating check > of, for example, an upper loop bound. OK. Now I can understand what you are worrying about. Thanks for your clarification. > You say "the intrinsified Objects.checkIndex seems to generate more code than > inlined if-statements". Can you present some examples of Java code and the > corresponding C2 generated assembler where this happens? Will do it later. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/2127
Re: RFR: 8259925: [Vector API] Unreasonable IndexOutOfBoundsException message when length < vlen
On Wed, 20 Jan 2021 19:30:41 GMT, Paul Sandoz wrote: > Unfortunately it is still problematic because you have replaced the intrinsic > check (that performed by `Object.checksIndex`, or more specifically > [here](https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/jdk/internal/util/Preconditions.java#L261)). > > Further, you have replaced the bounds check options, in place for > experimentation. We are not ready yet to collapse our options, further > analysis is required as bounds checks can be very sensitive in C2. > > I would be open to you adding a third case, so that you can analyze the > performance without perturbing the default behavior. I suspect the correct > fix is to make intrinsic `Objects.checkFromIndexSize` in a similar manner to > `Object.checksIndex`. Hi @PaulSandoz , Thanks for your kind review and comments. To be honest, I didn't get your first point. As for the example above, the intrinsified Objects.checkIndex seems to generate more code than inlined if-statements. So I'm confused why it's a problem to replace the intrinsic check. Did you mean an intrinsic is always (or for most cases) better than non-intrinc or inlined if-statements? And why? Could you please make it more clearer to us? Thanks. - PR: https://git.openjdk.java.net/jdk/pull/2127
Re: RFR: 8259925: [Vector API] Unreasonable IndexOutOfBoundsException message when length < vlen [v2]
> Hi all,
>
> For this reproducer:
>
> import jdk.incubator.vector.ByteVector;
> import jdk.incubator.vector.VectorSpecies;
>
> public class Test {
> static final VectorSpecies SPECIES_128 = ByteVector.SPECIES_128;
> static byte[] a = new byte[8];
> static byte[] b = new byte[8];
>
> public static void main(String[] args) {
> ByteVector av = ByteVector.fromArray(SPECIES_128, a, 0);
> av.intoArray(b, 0);
> System.out.println("b: " + b[0]);
> }
> }
>
> The following IndexOutOfBoundsException message (length -7) is unreasonable.
>
> Exception in thread "main" java.lang.IndexOutOfBoundsException: Index 0 out
> of bounds for length -7
>
> It might be better to fix it like this.
>
> Exception in thread "main" java.lang.IndexOutOfBoundsException: Index 0 out
> of bounds for length 0
>
> Thanks.
> Best regards,
> Jie
Jie Fu has updated the pull request incrementally with one additional commit
since the last revision:
Fix the performance issue
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/2127/files
- new: https://git.openjdk.java.net/jdk/pull/2127/files/ca39b482..3fd8b316
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=2127=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk=2127=00-01
Stats: 8 lines in 1 file changed: 6 ins; 0 del; 2 mod
Patch: https://git.openjdk.java.net/jdk/pull/2127.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2127/head:pull/2127
PR: https://git.openjdk.java.net/jdk/pull/2127
Re: RFR: 8259925: [Vector API] Unreasonable IndexOutOfBoundsException message when length < vlen
On Tue, 19 Jan 2021 21:40:24 GMT, Paul Sandoz wrote:
>> Hi all,
>>
>> For this reproducer:
>>
>> import jdk.incubator.vector.ByteVector;
>> import jdk.incubator.vector.VectorSpecies;
>>
>> public class Test {
>> static final VectorSpecies SPECIES_128 = ByteVector.SPECIES_128;
>> static byte[] a = new byte[8];
>> static byte[] b = new byte[8];
>>
>> public static void main(String[] args) {
>> ByteVector av = ByteVector.fromArray(SPECIES_128, a, 0);
>> av.intoArray(b, 0);
>> System.out.println("b: " + b[0]);
>> }
>> }
>>
>> The following IndexOutOfBoundsException message (length -7) is unreasonable.
>>
>> Exception in thread "main" java.lang.IndexOutOfBoundsException: Index 0 out
>> of bounds for length -7
>>
>> It might be better to fix it like this.
>>
>> Exception in thread "main" java.lang.IndexOutOfBoundsException: Index 0 out
>> of bounds for length 0
>>
>> Thanks.
>> Best regards,
>> Jie
>
> That change may cause performance issues. I would recommend leaving as is for
> now even through the error message is not great. Bounds checking is quite
> sensitive and WIP. Notice that we also have an option to call
> `Objects.checkFromIndexSize` which expresses the intent more accurately, but
> that is currently less optimal (at least it was when i last checked since it
> is non an intrinsic).
Thanks @PaulSandoz for your review and comments.
Updated:
- The performance issue has been fixed since there is no more operation for
common cases.
- The readability of OOB exception msg has been improved by following the
style of Objects.checkFromIndexSize.
- Less code generated (several blocks of code were optimized out for the
Test::test method below).
import jdk.incubator.vector.ByteVector;
import jdk.incubator.vector.VectorSpecies;
public class Test {
static final VectorSpecies SPECIES_128 = ByteVector.SPECIES_128;
static byte[] a = new byte[88];
static byte[] b = new byte[88];
public static void test() {
ByteVector av = ByteVector.fromArray(SPECIES_128, a, 0);
av.intoArray(b, 0);
}
public static void main(String[] args) {
for (int i = 0; i < 10; i++) {
test();
}
System.out.println("b: " + b[0]);
}
}
What do you think of it?
Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/2127
RFR: 8259925: [Vector API] Unreasonable IndexOutOfBoundsException message when length < vlen
Hi all,
For this reproducer:
import jdk.incubator.vector.ByteVector;
import jdk.incubator.vector.VectorSpecies;
public class Test {
static final VectorSpecies SPECIES_128 = ByteVector.SPECIES_128;
static byte[] a = new byte[8];
static byte[] b = new byte[8];
public static void main(String[] args) {
ByteVector av = ByteVector.fromArray(SPECIES_128, a, 0);
av.intoArray(b, 0);
System.out.println("b: " + b[0]);
}
}
The following IndexOutOfBoundsException message (length -7) is unreasonable.
Exception in thread "main" java.lang.IndexOutOfBoundsException: Index 0 out of
bounds for length -7
It might be better to fix it like this.
Exception in thread "main" java.lang.IndexOutOfBoundsException: Index 0 out of
bounds for length 0
Thanks.
Best regards,
Jie
-
Commit messages:
- 8259925: [Vector API] Unreasonable IndexOutOfBoundsException message when
length < vlen
Changes: https://git.openjdk.java.net/jdk/pull/2127/files
Webrev: https://webrevs.openjdk.java.net/?repo=jdk=2127=00
Issue: https://bugs.openjdk.java.net/browse/JDK-8259925
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.java.net/jdk/pull/2127.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/2127/head:pull/2127
PR: https://git.openjdk.java.net/jdk/pull/2127
Integrated: 8258584: java/util/HexFormat/HexFormatTest.java fails on x86_32
On Thu, 17 Dec 2020 12:11:33 GMT, Jie Fu wrote: > Hi all, > > java/util/HexFormat/HexFormatTest.java fails on x86_32 due to '-Xmx4G'. > The reason is that -Xmx4G is invalid maximum heap size for 32-bit platforms. > The current implementation only supports maximum 3800M on 32-bit systems [1]. > > I've tried to reduce the -Xmx size, but it still fails even with -Xmx2G. > So this test seems to be brittle on 32-bit platforms since 2G is already > larger than 3800M/2=1900M. > The fix just skips the test for 32-bit systems. > > Thanks. > Best regards, > Jie > > [1] > https://github.com/openjdk/jdk/blob/master/src/hotspot/os/posix/os_posix.cpp#L567 This pull request has now been integrated. Changeset: 1594372c Author:Jie Fu URL: https://git.openjdk.java.net/jdk/commit/1594372c Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod 8258584: java/util/HexFormat/HexFormatTest.java fails on x86_32 Reviewed-by: rriggs - PR: https://git.openjdk.java.net/jdk/pull/1817
Re: RFR: 8258584: java/util/HexFormat/HexFormatTest.java fails on x86_32 [v2]
On Mon, 21 Dec 2020 15:34:16 GMT, Roger Riggs wrote:
>> Jie Fu has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Ignore OOME for testOOME
>> - Revert the change
>
> Completely ignoring the exception will leave no trace that the test was
> skipped or why.
> Please retain the printing of the memory limits and instead of rethrowing the
> oome add:
> new SkipException("Insufficient Memory to test OOME");```
> (It will need an import of org.testng.SkipException).
> Throwing SkipException will flag the test as being skipped in the Jtreg
> summary.
> Completely ignoring the exception will leave no trace that the test was
> skipped or why.
> Please retain the printing of the memory limits and instead of rethrowing the
> oome add:
> `throw new SkipException("Insufficient Memory to test OOME");`
> (It will need an import of org.testng.SkipException).
> Throwing SkipException will flag the test as being skipped in the Jtreg
> summary.
Updated.
Thanks for your help.
-
PR: https://git.openjdk.java.net/jdk/pull/1817
Re: RFR: 8258584: java/util/HexFormat/HexFormatTest.java fails on x86_32 [v3]
> Hi all, > > java/util/HexFormat/HexFormatTest.java fails on x86_32 due to '-Xmx4G'. > The reason is that -Xmx4G is invalid maximum heap size for 32-bit platforms. > The current implementation only supports maximum 3800M on 32-bit systems [1]. > > I've tried to reduce the -Xmx size, but it still fails even with -Xmx2G. > So this test seems to be brittle on 32-bit platforms since 2G is already > larger than 3800M/2=1900M. > The fix just skips the test for 32-bit systems. > > Thanks. > Best regards, > Jie > > [1] > https://github.com/openjdk/jdk/blob/master/src/hotspot/os/posix/os_posix.cpp#L567 Jie Fu has updated the pull request incrementally with one additional commit since the last revision: Skip OOME - Changes: - all: https://git.openjdk.java.net/jdk/pull/1817/files - new: https://git.openjdk.java.net/jdk/pull/1817/files/38d4d01a..83cbdaaf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1817=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1817=01-02 Stats: 7 lines in 1 file changed: 6 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1817.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1817/head:pull/1817 PR: https://git.openjdk.java.net/jdk/pull/1817
Re: RFR: 8258584: java/util/HexFormat/HexFormatTest.java fails on x86_32 [v2]
On Thu, 17 Dec 2020 16:14:56 GMT, Roger Riggs wrote: > Disabling all of the tests on 32 bit is not a good idea. > > Instead the HexFormatTest.testOOME test should be skipped or the OOME should > be ignored. > Checking Runtime.getRuntime().maxMemory() should provide enough info to skip > it. Thanks @RogerRiggs for your review and comments. Let's ignore the OOME for testOOME. What do you think of the updated change? Thanks. - PR: https://git.openjdk.java.net/jdk/pull/1817
Re: RFR: 8258584: java/util/HexFormat/HexFormatTest.java fails on x86_32 [v2]
> Hi all, > > java/util/HexFormat/HexFormatTest.java fails on x86_32 due to '-Xmx4G'. > The reason is that -Xmx4G is invalid maximum heap size for 32-bit platforms. > The current implementation only supports maximum 3800M on 32-bit systems [1]. > > I've tried to reduce the -Xmx size, but it still fails even with -Xmx2G. > So this test seems to be brittle on 32-bit platforms since 2G is already > larger than 3800M/2=1900M. > The fix just skips the test for 32-bit systems. > > Thanks. > Best regards, > Jie > > [1] > https://github.com/openjdk/jdk/blob/master/src/hotspot/os/posix/os_posix.cpp#L567 Jie Fu has updated the pull request incrementally with two additional commits since the last revision: - Ignore OOME for testOOME - Revert the change - Changes: - all: https://git.openjdk.java.net/jdk/pull/1817/files - new: https://git.openjdk.java.net/jdk/pull/1817/files/6b32101d..38d4d01a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1817=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1817=00-01 Stats: 8 lines in 1 file changed: 0 ins; 6 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/1817.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1817/head:pull/1817 PR: https://git.openjdk.java.net/jdk/pull/1817
RFR: 8258584: java/util/HexFormat/HexFormatTest.java fails on x86_32
Hi all, java/util/HexFormat/HexFormatTest.java fails on x86_32 due to '-Xmx4G'. The reason is that -Xmx4G is invalid maximum heap size for 32-bit platforms. The current implementation only supports maximum 3800M on 32-bit systems [1]. I've tried to reduce the -Xmx size, but it still fails even with -Xmx2G. So this test seems to be brittle on 32-bit platforms since 2G is already larger than 3800M/2=1900M. The fix just skips the test for 32-bit systems. Thanks. Best regards, Jie [1] https://github.com/openjdk/jdk/blob/master/src/hotspot/os/posix/os_posix.cpp#L567 - Commit messages: - 8258584: java/util/HexFormat/HexFormatTest.java fails on x86_32 Changes: https://git.openjdk.java.net/jdk/pull/1817/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1817=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8258584 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/1817.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1817/head:pull/1817 PR: https://git.openjdk.java.net/jdk/pull/1817
