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

[Emanuel Peter]

On Wed, 11 Dec 2024 16:07:04 GMT, Quan Anh Mai <qamai at openjdk.org> wrote:

>> @jatin-bhateja Thanks for the bug report! I was able to reproduce it on my system as well. I apologize for the late response, I had been busy with my studies and only had a chance to look recently. It looks like this bug is caused because we run `Identity()` on all of the nodes, which finds new optimizations by replacing `LoadNode`s with a non-load identity. This causes an uncommon trap branch to be optimized out and the `HaltNode` replaced with a TOP `ConNode`. Usually these are removed by `RootNode::Ideal`, but since we don't do regular `Ideal` in lowering it ends up running into an assert later.
>> 
>> This is interesting because it suggests that there are potentially calls to `Identity()` that aren't run in earlier IGVN rounds. I didn't take a very in-depth look but it might be good to investigate separately. Because of that I'm not sure if we can rely on `Identity()` here, especially if there are identity transforms that rely on other `Ideal` calls. @merykitty do you have any thoughts on the best way to fix this?
>> 
>> @eme64 Ah, do you mean the recursive reduction op generation based on vector length? I think that could be handled here as well by splitting the reduction operation into multiple backend-specific nodes. It would still need to be implemented per-backend, but there could be benefits with optimizing more complex reductions, as you mention.
>
> @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.

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

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