On Wed, 4 Dec 2024 09:42:59 GMT, Aleksey Shipilev <sh...@openjdk.org> wrote:

>> Found this while cleaning up x86_32 code for removal.
>> 
>> In our current code there is a block added by 
>> [JDK-8076373](https://bugs.openjdk.org/browse/JDK-8076373):
>> https://github.com/openjdk/jdk/blob/3b21a298c29d88720f6bfb2dc1f3305b6a3db307/src/hotspot/share/compiler/compileBroker.cpp#L1451-L1473
>> 
>> Ostensibly, that block is for x86_32 handling of signalling NaNs -- x87 FPU 
>> has a peculiarity with them. See other funky bugs we seen with it: 
>> [JDK-8285985](https://bugs.openjdk.org/browse/JDK-8285985), 
>> [JDK-8293991](https://bugs.openjdk.org/browse/JDK-8293991).
>> 
>> But the way current block is coded, it is enabled for X86 wholesale, which 
>> also means x86_64! In fact, it is likely even worse on x86_64, because the 
>> related "fast" entries are generated only for x86_32:
>> https://github.com/openjdk/jdk/blob/3b21a298c29d88720f6bfb2dc1f3305b6a3db307/src/hotspot/share/interpreter/templateInterpreterGenerator.cpp#L493-L502
>> 
>> This can be solved by checking `IA32` instead of `X86`. This block would be 
>> gone completely once we remove x86_32 port. Meanwhile, we can make it right 
>> by x86_64, and make eventual x86_32 removal less confusing. This issue seems 
>> to only affect the compilation of native methods, while most of the hot code 
>> is riding on compiler intrinsics. I'll put performance data in comments.
>> 
>> Additional testing:
>>  - [x] Linux x86_64 server fastdebug, `all`
>
> Aleksey Shipilev has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Disable IR tests on IA32

I just launched testing, I don't think @vnkozlov did that already. Better to 
hold off until RDP1 / JDK25 fork anyway.

test/hotspot/jtreg/compiler/c2/irTests/TestFPConversion.java line 33:

> 31:  * @summary Test that code generation for FP conversion works as intended
> 32:  * @library /test/lib /
> 33:  * @requires os.arch != "x86" & os.arch != "i386"

It would have been preferrable to add this to the IR rule, so the test still 
runs elsewhere.
What about `aarch64`?

-------------

PR Review: https://git.openjdk.org/jdk/pull/22446#pullrequestreview-2478784069
PR Review Comment: https://git.openjdk.org/jdk/pull/22446#discussion_r1869595147

Reply via email to