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"

> [These cases](https://github.com/fg1417/jdk/actions/runs/13520045169/attempts/1#summary-37778338908) failed. Because, when setting the offset for `implicit null check`, it's assumed that all instruction clauses emitted by memory related machnodes are ended up with `ldr/str`. 

> I haven't found any determinative evidence to show that they will never be picked by `implicit null check`. Maybe we also need to fix them.

I just did an experiment, and I see that when `popCountI_mem` is used, it is guarded by an explicit null check, rather than using an implcit one. It seems that C2 carefully checks operands for implicit null check opportunities.

However, it seems that this patch is turning out to have a much larger blast radius than I expected. I was hoping that it'd just work, without affecting anything else. I don't think that this idea really justifies making such changes to C2, sorry.

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

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