RFR: JDK-8184900 AArch64: Fix overflow in immediate cmp instruction
Hi, all For a fastdebug build openjdk in jdk10/hs tree, there is a failed test case (TestOptionsWithRanges) in jtreg. The reason is that the immediate cmp instruction has 12 significant bits. This causes an overflow in TestOptionsWithRanges. With this patch, TestOptionsWithRanges can be passed and there aren't new failed test cases in jtreg. Bug: https://bugs.openjdk.java.net/browse/JDK-8184900 Webrev: http://cr.openjdk.java.net/~yzhang/8184900/webrev.00/ Please help to review it. Regards, Yang
On 25/07/17 05:46, Yang Zhang wrote:
Hi, all
For a fastdebug build openjdk in jdk10/hs tree, there is a failed test case (TestOptionsWithRanges) in jtreg. The reason is that the immediate cmp instruction has 12 significant bits. This causes an overflow in TestOptionsWithRanges.
With this patch, TestOptionsWithRanges can be passed and there aren't new failed test cases in jtreg.
Bug: https://bugs.openjdk.java.net/browse/JDK-8184900
Webrev: http://cr.openjdk.java.net/~yzhang/8184900/webrev.00/
Please help to review it.
This comment makes no sense to me: + // The imm should follow aarch64 add/sub imm12 constraint. inline void cmp(Register Rd, unsigned imm) { subs(zr, Rd, imm); } Please just say "imm is limited to 12 bits." inline void cmnw(Register Rd, unsigned imm) { addsw(zr, Rd, imm); } diff --git a/src/cpu/aarch64/vm/stubGenerator_aarch64.cpp b/src/cpu/aarch64/vm/stubGenerator_aarch64.cpp --- a/src/cpu/aarch64/vm/stubGenerator_aarch64.cpp +++ b/src/cpu/aarch64/vm/stubGenerator_aarch64.cpp @@ -764,7 +764,8 @@ // alignment. Label small; int low_limit = MAX2(zva_length * 2, (int)BlockZeroingLowLimit); - __ cmp(cnt, low_limit >> 3); + Register tmp = rscratch1; + __ subs(tmp, cnt, low_limit >> 3); Please don't declare Register tmp if it's only used once. Otherwise fine, thanks. -- Andrew Haley Java Platform Lead Engineer Red Hat UK Ltd. <https://www.redhat.com> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Hi Andrew I have updated the patch based on your feedback. http://cr.openjdk.java.net/~yzhang/8184900/webrev.01/ Please check it again. Regards Yang On 25 July 2017 at 16:34, Andrew Haley <aph@redhat.com> wrote:
On 25/07/17 05:46, Yang Zhang wrote:
Hi, all
For a fastdebug build openjdk in jdk10/hs tree, there is a failed test case (TestOptionsWithRanges) in jtreg. The reason is that the immediate cmp instruction has 12 significant bits. This causes an overflow in TestOptionsWithRanges.
With this patch, TestOptionsWithRanges can be passed and there aren't new failed test cases in jtreg.
Bug: https://bugs.openjdk.java.net/browse/JDK-8184900
Webrev: http://cr.openjdk.java.net/~yzhang/8184900/webrev.00/
Please help to review it.
This comment makes no sense to me:
+ // The imm should follow aarch64 add/sub imm12 constraint. inline void cmp(Register Rd, unsigned imm) { subs(zr, Rd, imm); }
Please just say "imm is limited to 12 bits."
inline void cmnw(Register Rd, unsigned imm) { addsw(zr, Rd, imm); } diff --git a/src/cpu/aarch64/vm/stubGenerator_aarch64.cpp b/src/cpu/aarch64/vm/stubGenerator_aarch64.cpp --- a/src/cpu/aarch64/vm/stubGenerator_aarch64.cpp +++ b/src/cpu/aarch64/vm/stubGenerator_aarch64.cpp @@ -764,7 +764,8 @@ // alignment. Label small; int low_limit = MAX2(zva_length * 2, (int)BlockZeroingLowLimit); - __ cmp(cnt, low_limit >> 3); + Register tmp = rscratch1; + __ subs(tmp, cnt, low_limit >> 3);
Please don't declare Register tmp if it's only used once.
Otherwise fine, thanks.
-- Andrew Haley Java Platform Lead Engineer Red Hat UK Ltd. <https://www.redhat.com> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
On 26/07/17 08:59, Yang Zhang wrote:
I have updated the patch based on your feedback.
http://cr.openjdk.java.net/~yzhang/8184900/webrev.01/
Please check it again.
My apologies. I didn't mean to say you'd have to submit it again, it's fine. Thanks. -- Andrew Haley Java Platform Lead Engineer Red Hat UK Ltd. <https://www.redhat.com> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Pushed. Thanks. Shall I do a backport for jdk8u-aarch64 repo? It has the same issue. JDK9 seems fine. On 26 July 2017 at 16:09, Andrew Haley <aph@redhat.com> wrote:
On 26/07/17 08:59, Yang Zhang wrote:
I have updated the patch based on your feedback.
http://cr.openjdk.java.net/~yzhang/8184900/webrev.01/
Please check it again.
My apologies. I didn't mean to say you'd have to submit it again, it's fine.
Thanks.
-- Andrew Haley Java Platform Lead Engineer Red Hat UK Ltd. <https://www.redhat.com> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
On 26/07/17 11:33, Felix Yang wrote:
Shall I do a backport for jdk8u-aarch64 repo? It has the same issue. JDK9 seems fine.
Do you know why there is such a difference between 8 and 9? -- Andrew Haley Java Platform Lead Engineer Red Hat UK Ltd. <https://www.redhat.com> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
I haven't look into the difference. But I can take a look if you want. On 26 July 2017 at 18:40, Andrew Haley <aph@redhat.com> wrote:
On 26/07/17 11:33, Felix Yang wrote:
Shall I do a backport for jdk8u-aarch64 repo? It has the same issue. JDK9 seems fine.
Do you know why there is such a difference between 8 and 9?
-- Andrew Haley Java Platform Lead Engineer Red Hat UK Ltd. <https://www.redhat.com> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
Hi, Here is the history: For jdk9: issue was fixed by: 8161190: AArch64: Fix overflow in immediate cmp instruction ( http://hg.openjdk.java.net/jdk9/jdk9/hotspot/rev/fbd5edf4d6e3). But this was not backport to jdk8u-aarch64. For jdk10: issue reintroduced by: 8179444: AArch64: Put zero_words on a diet (http://hg.openjdk.java.net/jdk10/jdk10/hotspot/rev/bd7fe2f1094d) and was fixed recently by http://hg.openjdk.java.net/jdk10/hs/hotspot/rev/23e687f0c874 For jdk8u-aarch64 repo, I'm backporting patch for 8161190: http://cr.openjdk.java.net/~fyang/8161190-backport/webrev.00/ Tested with JTreg hotspot, OK to push? Thanks, Felix On 26 July 2017 at 18:48, Felix Yang <felix.yang@linaro.org> wrote:
I haven't look into the difference. But I can take a look if you want.
On 26 July 2017 at 18:40, Andrew Haley <aph@redhat.com> wrote:
On 26/07/17 11:33, Felix Yang wrote:
Shall I do a backport for jdk8u-aarch64 repo? It has the same issue. JDK9 seems fine.
Do you know why there is such a difference between 8 and 9?
-- Andrew Haley Java Platform Lead Engineer Red Hat UK Ltd. <https://www.redhat.com> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
participants (3)
-
Andrew Haley
-
Felix Yang
-
Yang Zhang