RFR: 8312980: C2: "malformed control flow" created during incremental inlining

[Daniel D. Daugherty]

On Tue, 3 Oct 2023 05:10:37 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> When `testHelper()` is late inlined, `o` is recorded in the replaced
>> nodes because of the cast to `B` in that method. When late inlining
>> finishes, c2 tries to replace `o` with the resulting `CheckCastPP` to
>> `B` for uses dominated by the call. ``test` and `testHelper2` have 2
>> type checks for A: there are 2 `CheckCastPP` nodes pinned at 2
>> different controls (one is dominated by the `testHelper()` call, one
>> is not), a single `CmpP` that's shared by the 2 type checks (one of
>> the type check is dominated, the other is not). To check if a
>> replacement is legal, for each use of the node to be replaced, the
>> code in `ReplacedNodes::apply()` follow uses and uses of uses until it
>> finds a node that's a CFG node or is pinned. It then uses the IGVN
>> heuristics to figure out if the CFG is dominated by the call or
>> not. If it finds a CFG node that is not dominated, then that use is
>> skipped.
>> 
>> What happens here is that `ReplacedNodes::apply()` checks the control
>> input for each `CheckCastPP` and finds one to be dominated and the
>> other not. So it performs the replacement only for the one that's
>> dominated. For the shared `CmpP`, it follows uses until it reaches the
>> `If` nodes of the type checks. One is dominated. The other is not. So
>> it declares `CmpP` to not be dominated and skips it.
>> 
>> When IGVN runs next, the `CheckCastPP` that had its input replaced now
>> casts a `B` to a `A` which results to top but the check that `o` is of
>> type `A` that guards the type check still tests that `o` of type
>> `Object` is a `B`. That check doesn't constant fold. Replaced nodes
>> introduced an inconsistency. What we would have needed, is for both
>> the `CmpP` and `CheckCastPP` inputs to be changed. But that wasn't
>> possible because the `CmpP` is shared.
>> 
>> The fix I propose is a tweak to `ReplacedNodes::apply()` so it goes
>> depth first over the use of the node to be replaced (let's call it N)
>> and the use of its uses. When it hits a node that's pinned or a CFG,
>> it checks if it is dominated. If that is the case, the chain of use
>> that leads from N is cloned and the clone of the use of N gets the
>> improved input. That way if a node on that path is shared and used in
>> some CFG path that's not dominated, it is unaffected. We then let IGVN
>> cleans up extraneous clones.
>
> Hi Roland, I'm seeing new failures with `compiler/c2/TestVerifyIterativeGVN.java` and `-XX:-TieredCompilation -XX:+AlwaysIncrementalInline`:
> 
> 
> Missed Value optimization:
> dist dump
> ---------------------------------------------
>    1   27  ConI  === 0  [[ 22 22 114 275 145 95 280 136 125 ]]  #int:-1
>    1  638  CastII  === 629 195  [[ 593 640 244 244 145 275 260 220 247 ]]  #int:>=0:www !jvms: String::checkIndex @ bci:5 (line 4832) StringLatin1::charAt @ bci:3 (line 46) String::charAt @ bci:12 (line 1555) IPAddressUtil::scan @ bci:41 (line 472)
>    1  142  Region  === 142 283 141  [[ 142 152 143 144 145 ]] #reducible  !jvms: IPAddressUtil::scan @ bci:28 (line 472)
>    0  145  Phi  === 142 638 27  [[ 149 ]]  #int !jvms: IPAddressUtil::scan @ bci:28 (line 472)
> Current type:
> int
> Optimized type:
> int:>=-1:www
> #
> # A fatal error has been detected by the Java Runtime Environment:
> #
> #  Internal Error (/workspace/open/src/hotspot/share/opto/phaseX.cpp:1087), pid=4100110, tid=4100126
> #  assert(!failure) failed: Missed optimization opportunity in PhaseIterGVN
> #
> # JRE version: Java(TM) SE Runtime Environment (22.0) (fastdebug build 22-internal-2023-10-02-1419484.tobias.hartmann.jdk2)
> # Java VM: Java HotSpot(TM) 64-Bit Server VM (fastdebug 22-internal-2023-10-02-1419484.tobias.hartmann.jdk2, compiled mode, sharing, compressed oops, compressed class ptrs, g1 gc, linux-amd64)
> # Problematic frame:
> # V  [libjvm.so+0x152c41e]  PhaseIterGVN::verify_optimize() [clone .part.0]+0x69e
> #
> 
> 
> Best regards,
> Tobias

@TobiHartmann and/or @rwestrel  - What's the current status of this PR? We see intermittent
failures in Mach5 Tier[4-8] due to the issue so I'd love to quiet that down...

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

PR Comment: https://git.openjdk.org/jdk/pull/15905#issuecomment-1753577129