arm64: alternative implementation of atomics using CAS

[carenas]

Works only on CPUs that implement LSE, and which is available since armv8.1-a (--march=armv8-a+lse), like the M1.

First patch implement the feature, last one is just leftover code that might be useful while debugging and therefore optional

[zherczeg]

"atomic_store doesn't incorrectly set the atomic value if it changes while in the critical zone" - based on the discussion in #160 we should simply say, that any store between the load/store makes the operation undefined.

"the wrong value is provided as "expected" through temp_reg" - this already makes the operation undefined

There should be a test for reading data between the load/store. There should be a test for op_flags as well (an existing test can be extended though).

The atomic load/store must satisfy many rules, maybe creating a list of them would look better.

Do you want to work on this or shall I do it?

[carenas]

"atomic_store doesn't incorrectly set the atomic value if it changes while in the critical zone" - based on the discussion in #160 we should simply say, that any store between the load/store makes the operation undefined.

OK, but it should be important to note that might apply to both read and write operations, and not only to the memory address, but also its surrounding area.

Indeed, really old ARM cpu's which luckily we don't support, would invalidate locks if any write was done anywhere in memory[1]

"the wrong value is provided as "expected" through temp_reg" - this already makes the operation undefined

That needs clarification in the documentation as well, note that the fact that ARM, and any other
implementation that uses LL/SC would just happily ignore that the value doesn't match results in the store incorrectly succeeding as shown by the test, and which is why it is a bug against the specification that says that if the value doesn't match then the memory is left unchanged.

I would also argue that calling it "temp_reg" is deceiving, and should be instead "state_reg" or something that better identifies it's actual use.

There should be a test for reading data between the load/store. There should be a test for op_flags as well (an existing test can be extended though).

I would instead argue there should be no reads, so that there are no more surprises when all other CPUs are implemented.

Do you want to work on this or shall I do it?

Considering I might seem to be still confused on how this is to be used, it might be better if you do it.

[1] https://en.wikipedia.org/wiki/Load-link/store-conditional

[zherczeg]

OK, but it should be important to note that might apply to both read and write operations, and not only to the memory address, but also its surrounding area.

I would not limit it to an area. All memory writes, regardless of the address, makes the operation undefined. I cannot think any practical uses, where writing cannot be postponed after the store.

I would also argue that calling it "temp_reg" is deceiving, and should be instead "state_reg" or something that better identifies it's actual use.

The comment says: temp_reg must contain the value loaded into dst_reg by the sljit_emit_atomic_load operation (the store operation does not preserve the value of temp_reg)

I think "temp" emphasize that its value is not preserved, but I am open to any ideas. I don't like the "state" term, since it does not contain any status.

I would instead argue there should be no reads, so that there are no more surprises when all other CPUs are implemented.

This is certainly possible, but it would be strange, if reads would block a memory store.

[zherczeg]

Testing this should be elsewhere.

[zherczeg]

I think the patch is quite nice now!

You can add /**/ comments freely anywhere to describe things, for me that is better than DEVELOPER.

I realized one type of test is missing: the double load.

• Create a static global variable, should be far from the stack allocated buffer
• Load the global value with atomic load
• Load a local value value with atomic load
• Store a local value value with atomic store
  This should work. A missing store should have no side effects.

[carenas]

At least with ARM's LL/SC, a second load invalidates the reservation of the first, which IMHO would be also in line with the constrain to only support one atomic per thread that was documented.

[zherczeg]

LGTM. Thank you for doing all the changes