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

[Jasmine Karthikeyan]

On Thu, 12 Dec 2024 07:23:11 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> @jaskarth That's really interesting, it indeed seems like a missed transformation to me. I think adding an IGVN verification where we verify that no other transformation can take place after an IGVN transformation would catch this, what do you think @eme64 ?
>> 
>> For this PR, my idealist side tells me that you can try repeatedly applying IGVN until there is no further progress. It may be cheap since there should be few to none transformations left. If that sounds inelegant a possible approach is to refactor the `Identity` call into an `apply_identity` similar to `apply_ideal` where you can override it with a method that always returns the current node until the missed transformation is fixed. IMO in principle, there should not be cases where we legitimately depend on `Ideal` to be invoked after `Identity` like this case.
>
> @merykitty not sure if you are talking about `VerifyIterativeGVN`? So far we only verify `Value` optimizations, and even there we have to make some exceptions: the problem is that some optimization at node n can in some cases enable an optimization at some node n2 far away. This happens if n2's optimization does some graph walk that looks at more than just its direct neigbhours.
> 
> We have an RFE to extend this to `Ideal` and `Identity`, and maybe more:
> [JDK-8298951](https://bugs.openjdk.org/browse/JDK-8298951) Umbrella: improve CCP and IGVN verification
> (feel free to contact me if you want to work on one of these)
> 
> But the issue is that some optimizations just walk too far, so that we cannot reasonably expect for all those nodes to notify us back if they have a mutation. We cannot really affort to notify everybody, or repeat IGVN on the whole graph. The goal of IGVN was that it is basically linear. If you make some local change, this should only trigger IGVN on the local graph, and not necessarily on the whole graph. That would be a huge performance impact.
> 
> Still: for many optimizations we could probably verify that they are always done after IGVN.

@eme64 Thanks a lot for looking at the PR! You're correct, `identify_useful_nodes` does add the whole graph. Having a worklist would indeed be better, but the problem I ran into is that the lowerable nodes vary based on the CPU backend, which makes it difficult to track from shared code. There would need to be some way of querying the backend to decide which nodes should be lowered, which sounded error-prone unless it worked over the whole graph as well. I tested the runtime cost of the current implementation by running some programs with `-XX:+CITime`, and the cost that I observed was negligible.

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

PR Comment: https://git.openjdk.org/jdk/pull/21599#issuecomment-2550352798