RFR: JDK-8315279: Factor 'basic_plus_adr' out of PhaseMacroExpand and delete make_load/store [v3]

[Vladimir Ivanov]

On Thu, 31 Aug 2023 16:05:32 GMT, Cesar Soares Lucas <cslucas at openjdk.org> wrote:

>> I believe the factory methods for AddPNode should be in the AddPNode class. The make_load / make_store methods in PhaseMacroExpand can be refactored to instead just use the "make" methods from Load/Store classes.
>> 
>> Tested with tier1-3.
>
> Cesar Soares Lucas has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Use reference instead of pointer.

src/hotspot/share/opto/addnode.hpp line 181:

> 179:   }
> 180: 
> 181:   static Node* make(PhaseIterGVN& igvn, Node* base, int offset) {

The overloads do look confusing now. When seeing a call to a factory method, I expect to see all inputs to be separately initialized. With `basic_plus_adr` the intention was clearer.

I'm not against the refactoring, but it feels like it moves the code in the wrong direction. The root problem is that we can't use `GraphKit `during macro expansion, so we duplicated functionality there over time. (`PhaseIdealLoop` is another victim.)

A better way would be to use `GraphKit` directly. And when we discussed it last time [1], Roland mentioned it was already implemented in Valhalla repo. 

[1] https://mail.openjdk.org/pipermail/hotspot-compiler-dev/2020-August/039612.html

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15480#discussion_r1311898349