On Wed, 6 Apr 2022 00:46:01 GMT, Vladimir Kozlov <k...@openjdk.org> 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
