RFR: 8359678: C2: assert(static_cast<T1>(result) == thing) caused by ReverseBytesNode::Value() [v3]

[Manuel Hässig]

On Tue, 8 Jul 2025 07:40:02 GMT, Hannes Greule <hgreule at openjdk.org> wrote:

>> Fixes an assertion when passing an int larger than short/char to the corresponding reverseBytes method in a constant-folding scenario. By just using static_cast, we can ignore the upper bytes and just swap the lower bytes.
>> 
>> Using jasm, I added a test case that covers such inputs. It felt easier to test this way than the other scenarios mentioned in the bug report.
>> 
>> I also removed the redundant checked_cast calls from the int/long case; we already have the correct type there.
>> 
>> Please review. Thanks.
>
> Hannes Greule has updated the pull request incrementally with one additional commit since the last revision:
> 
>   re-add package, add methods to Run annotation

That exception is indeed a deeper problem. After some investigation, I filed [JDK-8362046](https://bugs.openjdk.org/browse/JDK-8362046). Essentially, the code generation for the byte reverse unsigned short intrinsic on aarch64 does not narrow the result. So your test already paid dividends by exposing this long-standing bug. Thank you!

Since this is a fix for a bug in JDK 25 and RDP2 is close, I would suggest that we uncomment the offending line and merge it. But a Reviewer should sign off on that since this is my first rampdown 😅

For those wondering, why the signed short case is not commented out: code generation on aarch64 will sign extend the result. So far, i have not been able to make that case crash. If I was not creative enough in my endeavours to produce the same behaviour and there is a bug, the RNG will expose it at some point.

test/hotspot/jtreg/compiler/c2/gvn/ReverseBytesConstantsTests.java line 93:

> 91:         Asserts.assertEQ(Short.reverseBytes((short) 0x8070), testS3());
> 92:         Asserts.assertEQ(Short.reverseBytes(C_SHORT), testS4());
> 93:         Asserts.assertEQ(ReverseBytesConstantsHelper.reverseBytesShort(C_INT), testS5());

Suggestion:

        // TODO: uncomment after integration of  JDK-8362046
        // Asserts.assertEQ(ReverseBytesConstantsHelper.reverseBytesShort(C_INT), testS5());

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

Changes requested by mhaessig (Committer).

PR Review: https://git.openjdk.org/jdk/pull/25988#pullrequestreview-3011799731
PR Review Comment: https://git.openjdk.org/jdk/pull/25988#discussion_r2201726167