RFR: 8271600: C2: CheckCastPP which should closely follow Allocate is sunk of a loop

[Christian Hagedorn]

On Mon, 23 Aug 2021 08:37:50 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:

> Thanks for the review, Christian.
> 
> I took a look at JDK-8272562. Your analysis looks reasonable, but I'm still curious whether the problem is specific to cast nodes. My understanding is the general problem stems from the fact that a computation may be sunk into an effectively dead path. When C2 can't prove that the branch is dead, the dying computation on that path corrupts the graph. Do we need to introduce a more robust solution to guarantee the corruption never happens?

I think it is generally a problem that C2 cannot prove that this branch is dead but data could be dying. But it looks like that data can only die when sinking nodes with `CastII` nodes which gives us the improved type information to trigger that. I could not think of/find another testcase where this could occur otherwise. So, going with your originally proposed fix would prevent the problem from happening but it still exists. Might be worth to further investigate why C2 cannot let control die, too, in this case.

> > Proposed fix is to keep such cast nodes intact. Moreover, I propose to disable the optimization for all flavors of cast nodes.
> > Cast nodes usually accompany runtime checks and I don't see much benefit in moving the cast node away while leaving the runtime check inside the loop.
> 
> That looks too conservative to me. It's not always the case that a cast is guarded by a test (the test could have been optimized out). Also, if a node is in a loop nest composed of 2 loops and can be sunk out of both loops, this would be done in 2 loop opts pass. First, from the inner loop and then from the outer loop. On the first pass, cast nodes are added to pin the node out of the inner loop. The change you propose would then prevent sinking out of the outer loop.

If we don't go with that fix, then it makes even more sense to revisit JDK-8272562 to find a better solution. I'll give it another go.

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

PR: https://git.openjdk.java.net/jdk/pull/5199