RFR: 8349479: C2: when a Type node becomes dead, make CFG path that uses it unreachable [v2]

[Emanuel Peter]

On Thu, 20 Mar 2025 08:16:20 GMT, Christian Hagedorn <chagedorn at openjdk.org> wrote:

>> Roland Westrelin has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   review
>
> Thanks Roberto for the evaluation! That looks promising.

I've discussed this with @chhagedorn in the office yesterday. Here what I remember from that conversation.

- This patch here solves some real issues: multiple conditions together are impossible, but it is difficult to prove that directly from the conditions, i.e. it would almost take some theorem prover to show that the branch is impossible. But sometimes the data nodes come to an empty type.
- There are also cases where we could have easily enabled constant folding for the branch, and we are just suffering from data nodes being smarter than CFG nodes. Your patch would "fix" these bugs, but then we still have a branch in the code that in theory could have been folded, but was not. This may be performance wise slightly suboptimal, and we probably will struggle to find these cases. But most likely, the performance there is not critical.
- There is a bit of a question about how much overhead adding in the HaltNodes produces. It seems on Roberto's benchmark there was no noticeable difference. That's good. Still, this is only one benchmark, and I'm wondering what could happen worst-case. If the graph was slowly dying from the bottom up, could it happen that we essencially gradually replace every CFG node with a HaltNode, as the dying slowly propagates up the whole graph? Maybe that's still not terrible enough to do something smarter yet. If we find such a case, we could still think about it later.
- Should we keep the HaltNodes in the graph? The question here is if we assume that:
  - these HaltNodes should never be taken, because the data nodes have proven that the path is impossible? Then we could actually just constant fold the if before the HaltNode.
  - these HaltNodes may be taken, because maybe there is a bug and only that led to the constant folding of the data node. Hitting a HaltNode would be a proof of a bug, and so we should keep them. It is better to crash the program than to continue in an inconsistent state down the wrong branch. Deopt would have been desirable, but restoring the state is probably near impossible.
-  And we have to be aware that we will now not learn about bugs the old "bad graph" way, but the graph will just be cleaned up with HaltNodes, which hides those bugs. Maybe that is ok, because those bugs are annoying and mostly happen in dead code anyway, so maybe we were wasting our time on those? Not sure, maybe there were also some real bugs mixed in there... and it would be a shame to miss those.

TLDR: I'm in favour of this patch. There are some risks, but the benefits are probably worth it. There would have to be some more documentation, probably.

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

PR Comment: https://git.openjdk.org/jdk/pull/23468#issuecomment-2742688738