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

[Christian Hagedorn]

On Thu, 20 Mar 2025 08:08:25 GMT, Roberto Castañeda Lozano <rcastanedalo 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...
>
>> * 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).
> 
> I measured C2 speed for this patch on top of jdk-25+14 vs jdk-25+14 using DaCapo23 on two different platforms and do not see any significant effect, see detailed results [here](https://github.com/user-attachments/files/19361310/C2-speed-jdk-25%2B14-vs-JDK-8349479.pdf).

We've also discussed this in our team meeting and we all agree on the need and usefulness of this patch. We should definitely move forward with it. Maybe it even has a measurable positive impact on footprint due to eagerly cutting off dead paths in IGVN (mentioned by @robcasloz offline).

Summarizing my thoughts about what's left to do or decide upon:
- Flag to disable this patch to detect inefficiencies like messing up Assertion Predicates. This should only be possibly done in stress jobs and in the future if 8275202 is disabled as well. The assumption is that we rarely crash when disabling this patch, so the overhead of looking into such issues is minimal (we could still remove the flag if we get too many false positive reports). I could, for example, then run my Assertion Predicate tests with that additional flag to still trigger the issues.
- Run some extended CI testing (we can do that).
- Maybe for tracking purposes file an RFE stating the idea to only run this patch on those cases where we cannot do something else to fold control. As you've already stated earlier, this is hard and might not even be possible. But maybe someone comes up with something in the future.

What do you think? I will also have a closer look at the actual code later this week.

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

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