RFR: 8342662: C2: Add new phase for backend-specific lowering [v2]

[Jasmine Karthikeyan]

On Mon, 21 Oct 2024 06:11:26 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

>> Jasmine Karthikeyan has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Address some changes from code review
>
> src/hotspot/share/opto/phaseX.cpp line 2277:
> 
>> 2275: 
>> 2276:   // Try to find an existing version of the same node
>> 2277:   Node* existing = _igvn->hash_find_insert(n);
> 
> I think it would be easier if you have a switch in `gvn` that says you passed the point of doing `Ideal`, moving forward you will probably want to have a `IdealLowering` to transform nodes during this phase. `Identity` I think is fine since it returns an existing node.

Ah, do you mean having a method in `Node` that holds the lowering code? I was originally planning on doing it this way, but I think it would pose some problems where certain nodes' `Lower()` methods would only be overridden on certain backends, which would become messy. One of my goals was to keep the lowering code separate from shared code, so new lowerings could be implemented by just updating the main `lower_node` function in the backend.
About GVN, I think it makes sense to do it in a separate phase because GVN is used quite generally whereas lowering is only done once. Since the `transform_old` function in IGVN is pretty complex as well, I think it's simpler to just implement `Value()` and GVN separately. Thinking on it more I think Identity is probably a good idea too, since as you mention it can't introduce new nodes into the graph. Mainly I wanted to avoid the case where `Ideal()` could fold a lowered graph back into the original form, causing an infinite loop.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21599#discussion_r1814136201