Re: RFR: 8282221: x86 intrinsics for divideUnsigned and remainderUnsigned methods in java.lang.Integer and java.lang.Long [v8]
On Fri, 8 Apr 2022 00:55:50 GMT, Srinivas Vamsi Parasa wrote: >> I have few comments. > > Hi Vladimir (@vnkozlov), > > Incorporated all the suggestions you made in the previous review and pushed a > new commit. > Please let me know if anything else is needed. > > Thanks, > Vamsi > @vamsi-parasa I got failures in new tests when run with `-XX:UseAVX=3 > -XX:+UnlockDiagnosticVMOptions -XX:+UseKNLSetting ` flags: > > ``` > # A fatal error has been detected by the Java Runtime Environment: > # > # SIGFPE (0x8) at pc=0x7f2fa8c674ea, pid=3334, tid=3335 > # > # JRE version: Java(TM) SE Runtime Environment (19.0) (fastdebug build > 19-internal-2022-04-08-0157190.vladimir.kozlov.jdkgit) > # Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug > 19-internal-2022-04-08-0157190.vladimir.kozlov.jdkgit, mixed mode, sharing, > tiered, compressed oops, compressed class ptrs, g1 gc, linux-amd64) > # Problematic frame: > # J 504% c2 compiler.intrinsics.TestLongUnsignedDivMod.testDivideUnsigned()V > (48 bytes) @ 0x7f2fa8c674ea [0x7f2fa8c672a0+0x024a] > # > ``` > > ``` > # A fatal error has been detected by the Java Runtime Environment: > # > # SIGFPE (0x8) at pc=0x7fb8c0c4fb18, pid=3309, tid=3310 > # > # JRE version: Java(TM) SE Runtime Environment (19.0) (fastdebug build > 19-internal-2022-04-08-0157190.vladimir.kozlov.jdkgit) > # Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug > 19-internal-2022-04-08-0157190.vladimir.kozlov.jdkgit, mixed mode, sharing, > tiered, compressed oops, compressed class ptrs, g1 gc, linux-amd64) > # Problematic frame: > # J 445 c2 compiler.intrinsics.TestIntegerUnsignedDivMod.divmod(III)V (23 > bytes) @ 0x7fb8c0c4fb18 [0x7fb8c0c4fae0+0x0038] > # > ``` Hi Vladimir (@vnkozlov), fixed it in the new commit, could you pls check? This is being caused by lack of control() node in udiv/umod related nodes. After adding the control() node, the tests are passing for me. Thanks for pointing this out! - PR: https://git.openjdk.java.net/jdk/pull/7572
Re: RFR: 8282221: x86 intrinsics for divideUnsigned and remainderUnsigned methods in java.lang.Integer and java.lang.Long [v8]
On Fri, 8 Apr 2022 16:39:31 GMT, Vladimir Kozlov wrote:
>> Hi Vladimir (@vnkozlov),
>>
>> Incorporated all the suggestions you made in the previous review and pushed
>> a new commit.
>> Please let me know if anything else is needed.
>>
>> Thanks,
>> Vamsi
>
> @vamsi-parasa I got failures in new tests when run with `-XX:UseAVX=3
> -XX:+UnlockDiagnosticVMOptions -XX:+UseKNLSetting ` flags:
>
> # A fatal error has been detected by the Java Runtime Environment:
> #
> # SIGFPE (0x8) at pc=0x7f2fa8c674ea, pid=3334, tid=3335
> #
> # JRE version: Java(TM) SE Runtime Environment (19.0) (fastdebug build
> 19-internal-2022-04-08-0157190.vladimir.kozlov.jdkgit)
> # Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug
> 19-internal-2022-04-08-0157190.vladimir.kozlov.jdkgit, mixed mode, sharing,
> tiered, compressed oops, compressed class ptrs, g1 gc, linux-amd64)
> # Problematic frame:
> # J 504% c2 compiler.intrinsics.TestLongUnsignedDivMod.testDivideUnsigned()V
> (48 bytes) @ 0x7f2fa8c674ea [0x7f2fa8c672a0+0x024a]
> #
>
>
>
> # A fatal error has been detected by the Java Runtime Environment:
> #
> # SIGFPE (0x8) at pc=0x7fb8c0c4fb18, pid=3309, tid=3310
> #
> # JRE version: Java(TM) SE Runtime Environment (19.0) (fastdebug build
> 19-internal-2022-04-08-0157190.vladimir.kozlov.jdkgit)
> # Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug
> 19-internal-2022-04-08-0157190.vladimir.kozlov.jdkgit, mixed mode, sharing,
> tiered, compressed oops, compressed class ptrs, g1 gc, linux-amd64)
> # Problematic frame:
> # J 445 c2 compiler.intrinsics.TestIntegerUnsignedDivMod.divmod(III)V (23
> bytes) @ 0x7fb8c0c4fb18 [0x7fb8c0c4fae0+0x0038]
> #
@vnkozlov The `uDivI_rRegNode` currently emits machine code equivalent to the
following Java pseudocode:
if (div < 0) {
// fast path, if div < 0, then (unsigned)div > MAX_UINT / 2U
// I don't know why this is so complicated, basically this is rax u>=
div ? 1 : 0
return (rax & ~(rax - div)) >>> (Integer.SIZE - 1);
} else {
// slow path, just do the division normally
return rax u/ div;
}
What I am suggesting is to leave the negative-divisor fast part to be
implemented in the IR and the macro assembler should only concern emitting the
division instruction and not worry about optimisation here.
Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/7572
Re: RFR: 8282221: x86 intrinsics for divideUnsigned and remainderUnsigned methods in java.lang.Integer and java.lang.Long [v8]
On Fri, 8 Apr 2022 00:55:50 GMT, Srinivas Vamsi Parasa wrote: >> I have few comments. > > Hi Vladimir (@vnkozlov), > > Incorporated all the suggestions you made in the previous review and pushed a > new commit. > Please let me know if anything else is needed. > > Thanks, > Vamsi @vamsi-parasa I got failures in new tests when run with `-XX:UseAVX=3 -XX:+UnlockDiagnosticVMOptions -XX:+UseKNLSetting ` flags: # A fatal error has been detected by the Java Runtime Environment: # # SIGFPE (0x8) at pc=0x7f2fa8c674ea, pid=3334, tid=3335 # # JRE version: Java(TM) SE Runtime Environment (19.0) (fastdebug build 19-internal-2022-04-08-0157190.vladimir.kozlov.jdkgit) # Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 19-internal-2022-04-08-0157190.vladimir.kozlov.jdkgit, mixed mode, sharing, tiered, compressed oops, compressed class ptrs, g1 gc, linux-amd64) # Problematic frame: # J 504% c2 compiler.intrinsics.TestLongUnsignedDivMod.testDivideUnsigned()V (48 bytes) @ 0x7f2fa8c674ea [0x7f2fa8c672a0+0x024a] # # A fatal error has been detected by the Java Runtime Environment: # # SIGFPE (0x8) at pc=0x7fb8c0c4fb18, pid=3309, tid=3310 # # JRE version: Java(TM) SE Runtime Environment (19.0) (fastdebug build 19-internal-2022-04-08-0157190.vladimir.kozlov.jdkgit) # Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 19-internal-2022-04-08-0157190.vladimir.kozlov.jdkgit, mixed mode, sharing, tiered, compressed oops, compressed class ptrs, g1 gc, linux-amd64) # Problematic frame: # J 445 c2 compiler.intrinsics.TestIntegerUnsignedDivMod.divmod(III)V (23 bytes) @ 0x7fb8c0c4fb18 [0x7fb8c0c4fae0+0x0038] # - PR: https://git.openjdk.java.net/jdk/pull/7572
Re: RFR: 8282221: x86 intrinsics for divideUnsigned and remainderUnsigned methods in java.lang.Integer and java.lang.Long [v8]
On Wed, 6 Apr 2022 00:46:01 GMT, Vladimir Kozlov wrote:
>> Srinivas Vamsi Parasa has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> add error msg for jtreg test
>
> I have few comments.
Hi Vladimir (@vnkozlov),
Incorporated all the suggestions you made in the previous review and pushed a
new commit.
Please let me know if anything else is needed.
Thanks,
Vamsi
> src/hotspot/cpu/x86/assembler_x86.cpp line 12375:
>
>> 12373: }
>> 12374: #endif
>> 12375:
>
> Please, place it near `idivq()` so you would not need `#ifdef`.
Made the change as per your suggestion.
> src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4568:
>
>> 4566: subl(rdx, divisor);
>> 4567: if (VM_Version::supports_bmi1()) andnl(rax, rdx, rax);
>> 4568: else {
>
> Please, follow our coding stile here and in following methods:
>
> if (VM_Version::supports_bmi1()) {
> andnl(rax, rdx, rax);
> } else {
Pls see the new commit which fixed the coding style.
> src/hotspot/cpu/x86/x86_64.ad line 8701:
>
>> 8699: %}
>> 8700:
>> 8701: instruct udivI_rReg(rax_RegI rax, no_rax_rdx_RegI div, rFlagsReg cr,
>> rdx_RegI rdx)
>
> I suggest to follow the pattern in other `div/mod` instructions: `(rax_RegI
> rax, rdx_RegI rdx, no_rax_rdx_RegI div, rFlagsReg cr)`
>
> Similar in following new instructions.
Pls see the new commit which fixed the pattern.
> test/hotspot/jtreg/compiler/intrinsics/TestIntegerDivMod.java line 55:
>
>> 53: dividends[i] = rng.nextInt();
>> 54: divisors[i] = rng.nextInt();
>> 55: }
>
> I don't trust RND to generate corner cases.
> Please, add cases here and in TestLongDivMod.java for MAX, MIN, 0.
You are right. Using an updated corner cases test revealed divide by zero crash
which was fixed. Please see the updated jtreg tests inspired by unsigned
divide/remainder tests in test/jdk/java/lang/Long/Unsigned.java and
test/jdk/java/lang/Integer/Unsigned.java.
-
PR: https://git.openjdk.java.net/jdk/pull/7572
Re: RFR: 8282221: x86 intrinsics for divideUnsigned and remainderUnsigned methods in java.lang.Integer and java.lang.Long [v8]
On Wed, 6 Apr 2022 00:46:01 GMT, Vladimir Kozlov wrote: > I have few comments. Thank you Vladimir (@vnkozlov) for suggesting the changes! Will incorporate the suggestions and push an update in few hours. - PR: https://git.openjdk.java.net/jdk/pull/7572
Re: RFR: 8282221: x86 intrinsics for divideUnsigned and remainderUnsigned methods in java.lang.Integer and java.lang.Long [v8]
On Tue, 5 Apr 2022 20:26:18 GMT, Vamsi Parasa wrote:
>> Optimizes the divideUnsigned() and remainderUnsigned() methods in
>> java.lang.Integer and java.lang.Long classes using x86 intrinsics. This
>> change shows 3x improvement for Integer methods and upto 25% improvement for
>> Long. This change also implements the DivMod optimization which fuses
>> division and modulus operations if needed. The DivMod optimization shows 3x
>> improvement for Integer and ~65% improvement for Long.
>
> Vamsi Parasa has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add error msg for jtreg test
I have few comments.
src/hotspot/cpu/x86/assembler_x86.cpp line 12375:
> 12373: }
> 12374: #endif
> 12375:
Please, place it near `idivq()` so you would not need `#ifdef`.
src/hotspot/cpu/x86/c2_MacroAssembler_x86.cpp line 4568:
> 4566: subl(rdx, divisor);
> 4567: if (VM_Version::supports_bmi1()) andnl(rax, rdx, rax);
> 4568: else {
Please, follow our coding stile here and in following methods:
if (VM_Version::supports_bmi1()) {
andnl(rax, rdx, rax);
} else {
src/hotspot/cpu/x86/x86_64.ad line 8701:
> 8699: %}
> 8700:
> 8701: instruct udivI_rReg(rax_RegI rax, no_rax_rdx_RegI div, rFlagsReg cr,
> rdx_RegI rdx)
I suggest to follow the pattern in other `div/mod` instructions: `(rax_RegI
rax, rdx_RegI rdx, no_rax_rdx_RegI div, rFlagsReg cr)`
Similar in following new instructions.
test/hotspot/jtreg/compiler/intrinsics/TestIntegerDivMod.java line 55:
> 53: dividends[i] = rng.nextInt();
> 54: divisors[i] = rng.nextInt();
> 55: }
I don't trust RND to generate corner cases.
Please, add cases here and in TestLongDivMod.java for MAX, MIN, 0.
-
Changes requested by kvn (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/7572
Re: RFR: 8282221: x86 intrinsics for divideUnsigned and remainderUnsigned methods in java.lang.Integer and java.lang.Long [v8]
On Tue, 5 Apr 2022 20:26:18 GMT, Vamsi Parasa wrote: >> Optimizes the divideUnsigned() and remainderUnsigned() methods in >> java.lang.Integer and java.lang.Long classes using x86 intrinsics. This >> change shows 3x improvement for Integer methods and upto 25% improvement for >> Long. This change also implements the DivMod optimization which fuses >> division and modulus operations if needed. The DivMod optimization shows 3x >> improvement for Integer and ~65% improvement for Long. > > Vamsi Parasa has updated the pull request incrementally with one additional > commit since the last revision: > > add error msg for jtreg test Marked as reviewed by sviswanathan (Reviewer). Looks good to me. You need one more review. @vnkozlov Could you please help review this patch? - PR: https://git.openjdk.java.net/jdk/pull/7572
Re: RFR: 8282221: x86 intrinsics for divideUnsigned and remainderUnsigned methods in java.lang.Integer and java.lang.Long [v8]
> Optimizes the divideUnsigned() and remainderUnsigned() methods in > java.lang.Integer and java.lang.Long classes using x86 intrinsics. This > change shows 3x improvement for Integer methods and upto 25% improvement for > Long. This change also implements the DivMod optimization which fuses > division and modulus operations if needed. The DivMod optimization shows 3x > improvement for Integer and ~65% improvement for Long. Vamsi Parasa has updated the pull request incrementally with one additional commit since the last revision: add error msg for jtreg test - Changes: - all: https://git.openjdk.java.net/jdk/pull/7572/files - new: https://git.openjdk.java.net/jdk/pull/7572/files/e97c6fbc..8047767c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7572=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7572=06-07 Stats: 41 lines in 2 files changed: 37 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/7572.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7572/head:pull/7572 PR: https://git.openjdk.java.net/jdk/pull/7572
