Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Is __sync_val_compare_and_swap on LoongArch64 implemented as an atomic operation?

Is __sync_val_compare_and_swap on LoongArch64 implemented as an atomic operation?

The cas-128 instruction is only supported from 3A6000, which is added within ISA1.1.
For 3A5000 which is ISA1.0, __sync_val_compare_and_swap will call the libatomic.

It means that this change will fix the build break, but there is a correctness problem at runtime on systems before 3A6000. The one place where this method is used requires the updates to be atomic.

It means that this change will fix the build break,

Yes, this is very clear.

but there is a correctness problem at runtime on systems before 3A6000. The one place where this method is used requires the updates to be atomic.

If calling the libatomic which implements this by mutex_lock is not correct, I think there might be a correctness problem.

If calling the libatomic which implements this by mutex_lock is not correct, I think there might be a correctness problem.

Add a TODO comment and open an issue about this?

If calling the libatomic which implements this by mutex_lock is not correct, I think there might be a correctness problem.

Add a TODO comment and open an issue about this?

OK, I will add a TODO and an issue later.
I have a question, why the libatomic using mutex_lock to emulate the cas-128bit instruction is not correct here.
I think using the libatomic is just lower performance.

why the libatomic using mutex_lock to emulate the cas-128bit instruction is not correct here.

The one and only place that calls PalInterlockedCompareExchange128 requires the update to be atomic:

It should be possible to create a variant of the calling code for platforms that are missing atomic 128-bit update.

but there is a correctness problem at runtime on systems before 3A6000.

Since LA64's NativeAOT support is new (still yet to pass all tests), maybe we can make 3A6000 as LA64 baseline for AOT?

Our default strategy is to have the same min supported hardware and OS versions for both regular CoreCLR and NAOT. You can certainly do something else for Loongarch, but you will have to explain these differences to Loongarch users.

Our default strategy is to have the same min supported hardware and OS versions for both regular CoreCLR and NAOT. You can certainly do something else for Loongarch, but you will have to explain these differences to Loongarch users.

Thanks.

I will add some TODO comments to explain, and try to add some compile arguments which only support the ISA1.1.

why the libatomic using mutex_lock to emulate the cas-128bit instruction is not correct here.

The one and only place that calls PalInterlockedCompareExchange128 requires the update to be atomic:

runtime/src/coreclr/nativeaot/Runtime/CachedInterfaceDispatch.cpp

Lines 69 to 70 in 3d9334c

	// Helper function for updating two adjacent pointers (which are aligned on a double pointer-sized boundary)
	// atomically.

. If the 128-bit update is not atomic, it will result into intermittent crashes.
It should be possible to create a variant of the calling code for platforms that are missing atomic 128-bit update.

Hi, @jkotas
I have some question about the CAS-128bit when updating and loading the data.

Are these codes related with the CAS-128-bit data?
If yes, is there some problem if the 128-bit data are loaded by two instructions rather than a 128-bit load instruction? When finished the first 64-bit pointer loading, the CAS-128-bit update the whole 128-bit date, then the second loading will get a new value but now the old value.

Yes, this code is related to the 128-bit CAS update. If the code sees a new value of the first pointer, it needs to see the new value of the second pointer as well in subsequent read to maintain correctness.

Yes, this code is related to the 128-bit CAS update. If the code sees a new value of the first pointer, it needs to see the new value of the second pointer as well in subsequent read to maintain correctness.

But now these code will hit the wrong value at the very low possibility.

Right, the changes in this PR do not maintain this invariant that will lead to intermittent crashes.

Right, the changes in this PR do not maintain this invariant that will lead to intermittent crashes.

I agree with you.
I just list this potential error.
It's best to discuss the 128-bit loading by a new issue or PR.

Fix included in #111086