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

[Roberto Castañeda Lozano]

On Thu, 12 Jun 2025 14:23:43 GMT, Manuel Hässig <mhaessig at openjdk.org> wrote:

>> src/hotspot/cpu/x86/peephole_x86_64.cpp line 288:
>> 
>>> 286:   bool is_spill = lea_derived_oop->in(AddPNode::Address) != decode->in(1) &&
>>> 287:                   lea_derived_oop->in(AddPNode::Address)->is_SpillCopy() &&
>>> 288:                   decode->in(1)->is_SpillCopy();
>> 
>> The logic around `is_spill` could be simplified by declaring pointers to the lea and decode address inputs and setting these either to their direct inputs or one level up in case of spilling. Something like this:
>> 
>> 
>>   Node* lea_address = lea_derived_oop->in(AddPNode::Address);
>>   Node* decode_address = decode->in(1);
>> 
>>   bool is_spill = lea_address != decode_address &&
>>                   lea_address->is_SpillCopy() &&
>>                   decode_address->is_SpillCopy();
>> 
>>   if (is_spill) {
>>     decode_address = decode_address->in(1);
>>     lea_address = lea_address->in(1);
>>   }
>> 
>>   // The leaP* and the decode must have the same parent. If we have a spill, they must have
>>   // the same grandparent.
>>   if (lea_address != decode_address) {
>>     return false;
>>   }
>> 
>>   (...)
>> 
>> 
>> 
>> This creates more opportunities for simplification below, e.g. rewiring the base address of the affected leas directly to `lea_address` (or `decode_address`).
>
> That makes a lot of things much clearer. I like it! Thank you for the suggestion! Incorporated in 3d6f897

Thanks, now that you have defined `lea_address` and `decode_address`, the logic that distinguishes between the spill/no spill cases can be further simplified by letting these pointers step over the spill node in the case of spilling:


  if (is_spill) {
    decode_address = decode_address->in(1);
    lea_address = lea_address->in(1);
  }


Then you can use them directly later on instead of conditionally using e.g. `decode_address` or `decode_address->in(1)` depending on the value of `is_spill`. Here a sketch of my suggested refactoring (not tested): https://github.com/openjdk/jdk/commit/2b75e85dbeedd380ab3ea02c64816c931b3dfe33. Feel free to apply it (or parts of it) if you agree that it makes the code more readable.

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

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