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

[Vladimir Ivanov]

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

>> 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
>
>> I expect to see all inputs to be separately initialized.
> 
> I'm not sure what you mean by this part. Can you elaborate a little here?

static Node* make(PhaseIterGVN& igvn, Node* base, Node* ptr, Node* offset) {
    return igvn.register_new_node_with_optimizer(new AddPNode(base, ptr, offset));
  }

vs

  static Node* make(PhaseIterGVN& igvn, Node* base, int offset) {
    return (offset == 0) ? base : make(igvn, base, base, (Node*) igvn.makecon(offset));
  }


IMO latter case adds more confusion than improves readability. Why not  unconditionally call `AddPNode::make(base, base, offset)` instead?

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

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