On Wed, 13 May 2026 16:24:15 GMT, Marc Chevalier <[email protected]> wrote:
>> That was discovered when working on the intrinsic for isAtomic. It turns out >> it is actually not needed, but the shape of the graph to trigger the issue >> needs to be complex enough not to be simplified too hard, but simple enough >> that optimization triggers. >> >> Let's see what happens. In this screenshot, let's look at >> <img width="2520" height="1935" alt="1 before" >> src="https://github.com/user-attachments/assets/e6e1d0d1-2034-49b6-948c-138c23f56d48"; >> /> >> Nodes `569 LoadB` and `573 LoadB` are loading the fields from >> `nullFreeAtomicArray2[1]`, in the inlined `Asserts.assertEquals`. Since they >> are strict final fields, they cannot be observed to be mutated, and we can >> rewire the memory input a lot higher. We get >> <img width="1601" height="1841" alt="2 after" >> src="https://github.com/user-attachments/assets/cec607fd-16e8-4462-881d-658a5200bd45"; >> /> >> >> >> The difficulty comes from the node `563 LoadN` that corresponds to the >> access `nullFreeAtomicArray2[1]`: we can climb up to the initial value of >> the `355 AllocateArray`. >> <img width="1750" height="910" alt="3 getting input" >> src="https://github.com/user-attachments/assets/0b0bb92a-4d7d-40c9-a24d-847245f8f52a"; >> /> >> >> >> Here, the default value is the value `initVal1`, which is `340 LoadN`, and >> the type is opaque here. Then, the graph becomes >> <img width="1719" height="777" alt="4 nope" >> src="https://github.com/user-attachments/assets/ad1aa005-a77a-4c26-a65c-651c48d82cf5"; >> /> >> >> then >> <img width="539" height="427" alt="5 nope" >> src="https://github.com/user-attachments/assets/b41c119e-e0a5-4fcb-8ee5-1a30ae718389"; >> /> >> >> >> and here, it is not clear that these `LoadB` are for strict final fields. >> That starts to be wrong. Later, during GCM, precedence edges from `538 >> MemBarStoreStore` to the `LoadB` nodes, creating a cycle inside the same >> block, and then, the graph becomes not schedulable. >> >> The fix is either not to subsume `563 LoadN` by `340 LoadN` (which is sad) >> because types don't match, or subsumes it by something correct. Here, we can >> just add a `CheckCastPPNode` under the initial value to subsumes by >> something correct, and all is fine again. >> >> Should we check that the initial value indeed has the right type? Yes, but >> not here, and it is worked on (see [JDK-8383820: [lworld] Several native >> methods of ValueClass class lack proper argument >> validation](https://bugs.openjdk.org/browse/JDK-8383820)). I've just added a >> debug only check and halt, it shall be enough for now. >> >> Thanks, >> Marc >> >> -... > > Marc Chevalier has updated the pull request incrementally with one additional > commit since the last revision: > > Specifying the dependency type Apparently, I just encountered a [similar issue in mainline](https://bugs.openjdk.org/browse/JDK-8382936) (screw you array polymorphism). ------------- PR Comment: https://git.openjdk.org/valhalla/pull/2432#issuecomment-4457758280
