> 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 > > --------- > - [x] I confirm that I make this contribution in ...
Marc Chevalier has updated the pull request incrementally with one additional commit since the last revision: Specifying the dependency type ------------- Changes: - all: https://git.openjdk.org/valhalla/pull/2432/files - new: https://git.openjdk.org/valhalla/pull/2432/files/3af692a6..c7638095 Webrevs: - full: https://webrevs.openjdk.org/?repo=valhalla&pr=2432&range=01 - incr: https://webrevs.openjdk.org/?repo=valhalla&pr=2432&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/valhalla/pull/2432.diff Fetch: git fetch https://git.openjdk.org/valhalla.git pull/2432/head:pull/2432 PR: https://git.openjdk.org/valhalla/pull/2432
