RFR: 8353041: NeverBranchNode causes incorrect block frequency calculation

[Dean Long]

On Tue, 8 Apr 2025 09:45:25 GMT, Roberto Castañeda Lozano <rcastanedalo at openjdk.org> wrote:

>> This fixes a quality of implementation issue for infinite loops using a NeverBranch node.  We need Block::succ_prob() to return 1.0f for the 100% successful back-edge so that block frequencies are computed correctly.  I also fixed Block_Stack::most_frequent_successor() to choose the correct successor.  I verified that this corrects the huge frequency ratio that was detected and clamped by JDK-8346888.
>> 
>> Currently this bug is labeled noreg-hard with no new regression test, as it's not obvious how to write such as test.
>
>> Currently this bug is labeled noreg-hard with no new regression test, as it's not obvious how to write such as test.
> 
> The only idea I can think of would be matching and asserting on the output of `-XX:+PrintCFGBlockFreq`, e.g. for the first test in `compiler/loopopts/TestPhaseCFGNeverBranchToGotoMain.java` I get the line
> 
>    Loop: 1  trip_count: 1000000 freq:      0
> 
> before this fix, and the line
> 
>    Loop: 1  trip_count: 1000000 freq: 900000
> 
> after the fix. You could assert that you expect a `freq` greater than 0 for every encountered loop, or something similar. But I guess such a test would be quite fragile.

@robcasloz , that's a good suggestion to use -XX:+PrintCFGBlockFreq to check the result, but I agree, writing a test based on it does seem fragile.  Given that this code rarely changes (the bug has existed for 15+ years before it was noticed), I would expect the cost of the test (maintenance to prevent false positives) would exceed its value in finding actual regressions in this code.  Are reviewers OK with pushing this as-is w/o a regression test?

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

PR Comment: https://git.openjdk.org/jdk/pull/24390#issuecomment-2787856914