RFR: 8020282: Generated code quality: redundant LEAs in the chained dereferences

[Manuel Hässig]

On Tue, 3 Jun 2025 17:44:07 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> ## Summary
>> 
>> On x86, chained dereferences of narrow oops at a constant offset from the base oop can use a `lea` instruction to perform the address computation in one go using the `leaP8Narrow`, `leaP32Narrow`, and `leaPCompressedOopOffset` matching rules. However, the generated code contains an additional `lea` with an unused result:
>> 
>> ; OptoAssembly
>> 03d     decode_heap_oop_not_null R8,R10
>> 041     leaq    R10, [R12 + R10 << 3 + #12] (compressed oop addressing) ; ptr compressedoopoff32
>> 
>> ; x86
>> 0x00007f1f210625bd:   lea    (%r12,%r10,8),%r8        ; result is unused
>> 0x00007f1f210625c1:   lea    0xc(%r12,%r10,8),%r10    ; the same computation as decode, but with offset
>> 
>> 
>> This PR adds a peephole optimization to remove such redundant `lea`s.
>> 
>> ## The Issue in Detail
>> 
>> The ideal subgraph producing redundant `lea`s, or rather redundant `decodeHeapOop_not_null`s, is `LoadN -> DecodeN -> AddP`, where both the address and base edge of the `AddP` originate from the `DecodeN`. After matching, this becomes
>> 
>> LoadN -> decodeHeapOop_not_null -> leaP*
>>     ______________________________Î
>> 
>> where `leaP*` is either of `leaP8Narrow`, `leaP32Narrow`, or `leaPCompressedOopOffset` (depending on the heap location and size). Here, the base input of `leaP*` comes from the decode. Looking at the matching code path, we find that the `leaP*` rules match both the `AddP` and the `DecodeN`, since x86 can fold this, but the following code adds the decode back as the base input to `leaP*`:
>> 
>> https://github.com/openjdk/jdk/blob/c29537740efb04e061732a700582d43b1956cff4/src/hotspot/share/opto/matcher.cpp#L1894-L1897
>> 
>> On its face, this is completely unnecessary if we matched a `leaP*`, since it already computes the result of the decode,  so adding the `LoadN` node as base seems like the logical choice. However, if the derived oop computed by the `leaP*` gets added to an oop map, this `DecodeN` is needed as the base for the derived oop. Because as of now, derived oops in oop maps cannot have narrow base pointers.
>> 
>> This leaves us with a handful of possible solutions:
>>  1. implement narrow bases for derived oops in oop maps,
>>  2. perform some dead code elimination after we know which oops are part of oop maps,
>>  3. add a peephole optimization to simply remove unused `lea`s.
>> 
>> Option 1 would have been ideal in the sense, that it is the earliest possible point to remove the decode, which would simplify the graph and reduce pressure on the regi...
>
> src/hotspot/cpu/x86/peephole_x86_64.cpp line 270:
> 
>> 268: // away, this peephole can als recognize the decode as redundant and also remove the spill copy
>> 269: // if that is only used by the decode.
>> 270: bool Peephole::lea_remove_redundant(Block* block, int block_index, PhaseCFG* cfg_, PhaseRegAlloc* ra_,
> 
> Why do you need `_` suffix?

I don't really need them. I only matched the signature of the other peephole functions.

We could remove the underline for all peepholes.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25471#discussion_r2126104773