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

[Christian Hagedorn]

On Fri, 14 Mar 2025 13:33:31 GMT, Roland Westrelin <roland at openjdk.org> wrote:

> Capturing i >= 0 in the loop Phi or array address CastII or ConvI2L then enables better use of address modes on x86.

That's very promising.

> Except, narrowing the type of the Phi or CastII expose sC2 to the exact bug this PR tries to fix: what if the loop becomes unreachable but C2 can't fold it away and the Phi or CastII end up having an out of range input?

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. 

A develop flag to turn this patch off could also help but then we have the problem that someone uses the flag and reports an assertion failure that is actually not a real bug because it's one of these kinds of failures we cannot fix otherwise. But I assume it's quite rare that this will happen.

> For the test case that I added for this bug, the issue is that some CastII transformations widen the types of some nodes. I suppose the way to fix this would be to restrict those transformations so widening doesn't happen in some cases. It's going to be tricky (because widening happens so mostly identical CastII nodes can be commoned to improve code quality) and fragile (if to preserve performance, we choose to only restrict those transformations to few targeted cases).

It sounds hard to find a control path removal fix for these kind of issues. Also for the type being zero on the div by zero failing path which lets some type nodes die and control is not because we don't have an "everything but zero" type.

 > For 8275202, what I tried doing is that when the new pass proves a condition constant, rather than constant fold the condition, it mark the test as always failing/succeeding (so (If (Bool ...))is transformed into(If (Opaque4 (Booland theOpaque4captures the final result of theBool. Then the Opaque4` constant folds later. I found several issues with this:
 
Sounds interesting but as you've stated creates new problems.

Summarizing my thoughts:
- I'm in favor of this patch as a last resort solution for these seemingly unsolvable cases and all future problems.
- The unsolvable cases with control not being folded should be documented somewhere - maybe they can be solved in the future.
- (Maybe) being able to turn this patch off with a develop flag. That could also allow us to test patches that tackle some of these hard to solve cases at some point.
- We should make sure that compilation speed is not significantly affected by doing this search on all dying `Type` nodes (maybe @robcasloz can give you some pointers here - he did some compilation time measurements before).

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

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