RFR: 8341611: [REDO] AArch64: Clean up IndOffXX type and let legitimize_address() fix out-of-range operands [v3]

[Andrew Haley]

On Tue, 25 Feb 2025 11:26:33 GMT, Fei Gao <fgao at openjdk.org> wrote:

>> `IndOffXX` types don't do us any good. It would be simpler and faster to match a general-purpose `IndOff` type then let `legitimize_address()` fix any out-of-range operands. That'd reduce the size of the match rules and the time to run them.
>> 
>> This patch simplifies the definitions of `immXOffset` with an estimated range. Whether an immediate can be encoded in a `LDR`/`STR` instructions as an offset will be determined in the phase of code-emitting. Meanwhile, we add necessary `legitimize_address()` in the phase of matcher for all `LDR`/`STR` instructions using the new `IndOff` memory operands (fix [JDK-8341437](https://bugs.openjdk.org/browse/JDK-8341437)).
>> 
>> After this clean-up, memory operands matched with `IndOff` may require extra code emission (effectively a `lea`) before the address can be used. So we also modify the code about looking up precise offset of load/store instruction for implicit null check (fix [JDK-8340646](https://bugs.openjdk.org/browse/JDK-8340646)). On `aarch64` platform, we will use the beginning offset of the last instruction in the instruction clause emitted for a load/store machine node. Because `LDR`/`STR` is always the last one emitted, no matter what addressing mode the load/store operations finally use.
>> 
>> Tier 1 - 3 passed on `Macos-aarch64` with or without the vm option `-XX:+UseZGC`.
>
> Fei Gao has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains four commits:
> 
>  - Merge branch 'master' into cleanup_indoff
>  - Update the copyright year and code comments
>  - Merge branch 'master' into cleanup_indoff
>  - 8341611: [REDO] AArch64: Clean up IndOffXX type and let legitimize_address() fix out-of-range operands
>    
>    IndOffXX types don't do us any good. It would be simpler and
>    faster to match a general-purpose IndOff type then let
>    legitimize_address() fix any out-of-range operands. That'd
>    reduce the size of the match rules and the time to run them.
>    
>    This patch simplifies the definitions of `immXOffset` with an
>    estimated range. Whether an immediate can be encoded in a
>    LDR/STR instructions as an offset will be determined in the phase
>    of code-emitting. Meanwhile, we add necessary
>    `legitimize_address()` in the phase of matcher for all LDR/STR
>    instructions using the new `IndOff` memory operands
>    (fix JDK-8341437).
>    
>    After this clean-up, memory operands matched with `IndOff` may
>    require extra code emission (effectively a lea) before the address
>    can be used. So we also modify the code about looking up precise
>    offset of load/store instruction for implicit null check
>    (fix JDK-8340646). On aarch64 platform, we will use the beginning
>    offset of the last instruction in the instruction clause emitted
>    for a load/store machine node. Because LDR/STR is always the last
>    one emitted, no matter what addressing mode the load/store
>    operations finally use.
>    
>    Tier 1 - 3 passed on Macos-aarch64 with or without the vm option
>    "-XX:+UseZGC"

The reason being: this patch cleans up and simplifies memory ops on AArch64. That's nice, but it doesn't fix any bug. If we could do it in a self-contained way, and it's beginning to look that we can't, then that would be fine. But I don't think it's worth the possibility of regressions. I've seen too many "cleanups" that have broken things.

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

PR Comment: https://git.openjdk.org/jdk/pull/22862#issuecomment-2685126429