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

[Christian Hagedorn]

On Fri, 20 Aug 2021 12:02:25 GMT, Vladimir Ivanov <vlivanov at openjdk.org> wrote:

> `PhaseIdealLoop::try_sink_out_of_loop()` tries to move nodes outside a loop. It doesn't work well for cast nodes which accompany `Allocate` nodes:  the cast reifies the effect instance initialization and turns a raw pointer into an oop. Once the cast node doesn't immediately follow the corresponding `Initialize`, it creates a window where the raw pointer is observed. And it can become so wide that the raw pointer crosses a safepoint and has to be recorded in the oop map (that's the case when the reported assertion failure happens). 
> 
> 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.  
> 
> Testing: hs-tier1 - hs-tier6

The fix looks reasonable to me.

Actually, on a separate note, I had the very same fix in mind for [JDK-8272562](https://bugs.openjdk.java.net/browse/JDK-8272562) where `CastII` nodes are causing problems instead of `CheckCastPP` nodes. In the attached testcase of that bug we sink a (`AddI` -> `CastII` (range [0, maxint]) -> `ConvI2L` (range [0, maxint]) -> ...) chain to an uncommon path of a div by zero check (the `CastII` node is a left over from an already removed range check). After sinking the nodes we have a chain `CastII` -> `AddI` -> `ConvI2L` -> ... (the sunk `CastII` node is removed in this process):

![sink_folding](https://user-images.githubusercontent.com/17833009/130252962-63de5b99-27a4-4d2a-831a-b77f86e66fa4.png)

Since these sunk nodes are pinned to the `493 IfFalse` path of the div by zero check `484 If`, we get additional type information for `545 CastII` in `CastII::Value()` that the type can be improved to `[0,0]` (because `489 Phi` must be zero on this path). `133 ConI` is -1. This means that the type of `544 AddI` becomes -1 which is out of the range [0, maxint] of `538 ConvI2L`. `538 ConvI2L` is replaced with top and we are folding away all the nodes up to the `499 Phi` which is then removed which eventually leads to a bad graph assertion failure. If we would just prevent any Constraint nodes to be sunk then we do not run into this problem. 

So, your patch also fixes JDK-8272562 even though it manifests differently. Not sure how to proceed with JDK-8272562 as it's not an actual duplicate but coincidentally requires the same fix. Should I just open a PR with the test only after this patch goes in or do you want to merge my [testcase](https://bugs.openjdk.java.net/secure/attachment/96032/Reduced.java) into your patch?

Thanks,
Christian

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

Marked as reviewed by chagedorn (Reviewer).

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