RFR: 8346664: C2: Optimize mask check with constant offset [v24]

[Quan Anh Mai]

On Fri, 28 Feb 2025 06:54:52 GMT, Matthias Ernst <duke at openjdk.org> wrote:

>> About severity: As long as we find and integrate a fix during `JDK25` it is fine (the issue does not break the CI that badly at the moment). If we get close to rampdown, then we have to consider if we want to backout 8346664 or if we defer the bug to `JDK26`.
>
> The good news: it's easy to reproduce the issue by adding a loop to CheckProp, and I have a fix up for CCP that appears(!) straightforward. I do not quite understand the interplay with IGVN, the set of "push" rules does not look consistent between the two phases to me. But re-reading @eme64's response, CCP is necessary for correctness, IGVN is optimization potential(?).
> 
> That said, I have taken up new employment and need to sort out the contribution framework first before I can contribute the fix.

@mernst-github IGVN and CCP try to infer information about a node from its inputs. As a result, when an input of a node is changed by IGVN or CCP, you need to do the inference on that node again. Theoretically, you need to push all nodes below a changed node to the worklist, but doing so is expensive and unnecessary. So, we use the fairly convoluted approach of pushing all immediate uses, and indirect uses are pushed in an ad-hoc manner depending on the way particular nodes do their inference. For example, in this case, since AndNode::Value looks through a LShiftNode, you need to do the opposite when pushing them to the IGVN worklist, i.e. pushing all AndNode that is a use of a direct use LShiftNode of the current node.

> I do not quite understand the interplay with IGVN, the set of "push" rules does not look consistent between the two phases to me.

It is because IGVN uses Ideal, Identity, and Value, which means that the pushing logic needs to take into consideration the inference of all these methods. While CCP only uses Value, so the pushing logic only needs to know which nodes the Value methods try to look through.

> CCP is necessary for correctness, IGVN is optimization potential(?).

Not really, we assume a graph is stable after IGVN, so missed idealisation here can also be considered a bug (especially missed transformations with CFG nodes). The difference is that these assumptions do not practically cover all nodes so we may get away with some of them.

> That said, I have taken up new employment and need to sort out the contribution framework first before I can contribute the fix.

Don't worry, RDP1 of JDK-25 is not so urgent. So take your time.

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

PR Comment: https://git.openjdk.org/jdk/pull/22856#issuecomment-2689917878