On Wed, 13 May 2026 13:06:52 GMT, Quan Anh Mai <[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 >> >> -... > > src/hotspot/share/opto/library_call.cpp line 4781: > >> 4779: } >> 4780: #endif >> 4781: init_val = _gvn.transform(new CheckCastPPNode(control(), >> init_val, TypeOopPtr::make_from_klass(array_klass->element_klass()))); > > Please always remember to set the `dependency` argument of the constructors > of `ConstraintCastNode`'s. It should only be `FloatingNarrowing` if we also > just emit the corresponding check for that cast, otherwise, it must be > `NonFloatingNarrowing`. I tihnk in this case, it should rather be `NonFloatingNarrowing`. I'm running tests again. Shouldn't this argument be made non-optional if we should always set it? ------------- PR Review Comment: https://git.openjdk.org/valhalla/pull/2432#discussion_r3234641630
