On Fri, 12 Sep 2025 16:06:28 GMT, Marc Chevalier <[email protected]> wrote:
>> As suspected, it's just a too big offset. We used to have a >> https://github.com/openjdk/valhalla/blob/c8d4a247861052aa6ed43125bcbe49995326938f/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp#L6133 >> >> I changed it into >> https://github.com/openjdk/valhalla/blob/880ae47831ed7262a0d3b30b92c3645bc67e5df2/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp#L6136 >> >> But >> [`ldp`](https://developer.arm.com/documentation/ddi0602/2025-06/Base-Instructions/LDP--Load-pair-of-registers-) >> has only a 7-bit offset. It's big, but not big enough. In some cases I've >> looked at, the offset can be as big as `536` which fits on 9 bits. >> [`ldr`](https://developer.arm.com/documentation/ddi0602/2025-06/Base-Instructions/LDR--immediate---Load-register--immediate--) >> supports a 9-bit offset. Let's change the `ldp` into >> >> >> ldr(rscratch1, Address(sp, sp_inc_offset)) >> ldr(rfp, Address(sp, sp_inc_offset + wordSize)) >> >> which will be merged into one `ldp` if the offset fits. >> >> But what if the offset is bigger than what fits on 9 bits? Well, us used to >> have the `ldr(rscratch1, Address(sp, sp_inc_offset))` so either we have a >> big problem (too big frames?) or `sp_inc_offset` was just bordeline and >> `sp_inc_offset + wordSize` is too big. But we still have `sp_inc_offset + >> wordSize == initial_framesize - 2 * wordSize` which would mean that >> `initial_framesize` doesn't fit on 9 bits either. Once again, that sounds >> like a bigger (and unlikely) problem. >> >> Thanks, >> Marc > > Marc Chevalier has updated the pull request incrementally with one additional > commit since the last revision: > > Add test Otherwise, looks good to me! test/hotspot/jtreg/compiler/valhalla/inlinetypes/RepairStackWithBigFrame.java line 35: > 33: * increment and rfp at the same time, since it only has a 7 bit > offset. > 34: * We use two ldr with 9-bit offsets instead. > 35: * @library /test/lib / Do you really need this? You do not seem to use anything from the library. test/hotspot/jtreg/compiler/valhalla/inlinetypes/RepairStackWithBigFrame.java line 43: > 41: * > -XX:CompileCommand=compileonly,compiler.valhalla.inlinetypes.RepairStackWithBigFrame::test > 42: * compiler.valhalla.inlinetypes.RepairStackWithBigFrame > 43: * @run main/othervm compiler.valhalla.inlinetypes.RepairStackWithBigFrame Can be changed to `main`: Suggestion: * @run main compiler.valhalla.inlinetypes.RepairStackWithBigFrame ------------- Marked as reviewed by chagedorn (Committer). PR Review: https://git.openjdk.org/valhalla/pull/1575#pullrequestreview-3217539332 PR Review Comment: https://git.openjdk.org/valhalla/pull/1575#discussion_r2344704684 PR Review Comment: https://git.openjdk.org/valhalla/pull/1575#discussion_r2344702071
