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

[Christian Hagedorn]

On Fri, 21 Mar 2025 16:25:14 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> > But also a problem, indeed. I just think that going into the future, we should still make a reasonable effort to try and let the control path die sanely without needing this patch. It should only serve as a last resort to avoid breaking the graph. While I think it's the safest solution, my concern is that we will not find inefficiencies anymore with this patch. For example, if someone breaks Assertion Predicates, how can we detect this when the graph will always be sane? It's especially tricky now that I'm still adding Assertion Predicate patches and things might break during development and it goes unnoticed. But maybe I just need to turn this patch off locally.
> 
> I agree with that. So ideally the code for this patch should only execute for those cases not properly handled some other way. I tried to figure out a way to do that but concluded it was not really possible. One thing could be to have a flag on `Compile` that's only set to true once a dangerous transformation is performed (in the case of this test case, some transformation involving cast nodes that widens the type at some point in the graph). The new logic would only execute when that flag is true. Do you think it's worth trying or would the logic still run too often to catch bugs elsewhere?

I guess it could be worth if these dangerous transformations are not executed too often or if we can efficiently have checks to only apply the patch to these problematic cases. But I see that it's probably quite difficult to detect those and we may end up running with the patch enabled in a lot of cases to be on the safe side and shadowing issues we could address and fix easily. 

So, I'm not sure how much effort we should put into that or if we should just have a global flag to disable this patch (enabled by default) and have some stress job that runs with the patch disabled. There is a risk of having false positive reports with that. But given how rarely we see a broken graph today traced back to these unsolvable cases, it might be justified. And we could still remove the flag again if we see many reports with this flag disabled. 

The story might be different with JDK-8275202 in and we could get a lot more reports with the flag disabled. But I assume you have a special flag guarding the new optimizations of 8275202. We could then just enforce the flag to enable this patch to be true when the optimizations of 8275202 are enabled with the dedicated flag.

> I think we want to leave the `Halt` nodes in the final code. Investigating crashes when compile code executes is somewhat trickier than crashes when compiling and that's a drawback of this patch. If the `Halt` nodes are removed then, in case of a bug where a path that's expected unreachable is taken, execution could proceed and fail only much later. That would lead to much harder and mysterious bugs.
> 
> Also it's not guaranteed that there's a an `If` right before the `Cast` or that that `If` is actually the condition guarding the `Cast`.

Sounds like it's safer to just sanely crash with a `halt` instead of executing unreachable paths by mistake which could have an unpredictable behavior.

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

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