[SYCL] Remove broken atomic_memory_order* tests

[t4c1]

Removes atomic_memory_order* tests, which are broken. They are using the pattern:

            auto ld = aar.load();
            ld += 1;
            aar.store(ld);

and checking if the whole sequence of operations is atomic. Which it is not - only each of the operations (load/store) on its own is atomic.

Before intel/llvm#4853 was merged no backend supported acquire release or sequentially consistent memory orders, so these tests were never run before.

This issue was first discussed here: intel/llvm#4853 (comment)

@bader @steffenlarsen

[steffenlarsen]

I believe you are right. I must have been confused in the moment because these are fundamentally wrong.

That said, I am not sure I am happy just removing these tests and leaving the functionality untested. Do you have an idea on how to correctly test the functionality?

Also, I am not sure I understand the motivation for fully removing some of the tests here while just removing the contents of others?

[t4c1]

That said, I am not sure I am happy just removing these tests and leaving the functionality untested.

Atomic loads and stores have other tests.

Do you have an idea on how to correctly test the functionality?

If you mean testing for correct memory orders - yes that is possible, just a lot of work because we would probably need a different test for each operation. If we are doing it, we should test all operations, not just loads and stores. Also, as with most tests for atomics, they would not be guaranteed to fail if run with incorrect implementation.

Also, I am not sure I understand the motivation for fully removing some of the tests here while just removing the contents of others?

All tests that actually run atomics are removed. The only thing I left in is the assert that if a device supports acquire release order it should also support acquire and release.

[steffenlarsen]

If you mean testing for correct memory orders - yes that is possible, just a lot of work because we would probably need a different test for each operation. If we are doing it, we should test all operations, not just loads and stores.

Yes, I meant memory ordering specifically. I am not expecting full covered - that's what we have the CTS for - but just some small example tests showing that devices that claim to support acq_rel and/or seq_cst can at least correctly handle that test.

Also, as with most tests for atomics, they would not be guaranteed to fail if run with incorrect implementation.

Of course. We just need something that could show us potential failures so we can fix them early.

All tests that actually run atomics are removed. The only thing I left in is the assert that if a device supports acquire release order it should also support acquire and release.

Then I think we should keep SYCL/AtomicRef/atomic_memory_order_seq_cst.cpp too.

[t4c1]

small example tests showing that devices that claim to support acq_rel and/or seq_cst can at least correctly handle that test.

I am not sure such a test for just one operation makes much sense. Whether a device supports certain memory order is usually documented somewhere. Such a test would in my opinion mostly catch errors connected to whether the implementation of atomic operation uses the correct order.

Then I think we should keep SYCL/AtomicRef/atomic_memory_order_seq_cst.cpp too.

What in this test would you like to keep? I don't see any assertion that if a device supports sequentially consistent order it should also support some other order.

[steffenlarsen]

Such a test would in my opinion mostly catch errors connected to whether the implementation of atomic operation uses the correct order.

Exactly, and I think that's a good test to have. If it states that it supports the memory order it should also use it correctly when requested. If it doesn't then that's a bug that should be found and fixed.

What in this test would you like to keep? I don't see any assertion that if a device supports sequentially consistent order it should also support some other order.

Apologies, I misunderstood what you meant.

[t4c1]

Exactly, and I think that's a good test to have.

I agree. What I am trying to argue is that if we are going into that, we should do such tests for all atomic operations.

[steffenlarsen]

I agree. What I am trying to argue is that if we are going into that, we should do such tests for all atomic operations.

I don't disagree, but for a start it would be nice to just have a basic case for the sake of not leaving it untested.

[bader]

I think it makes sense to create an issue to cover this functionality properly.

[bader]

@steffenlarsen, these tests fail in pre-commit, so it would be great to fix them ASAP. Are you okay to commit it "as is" and write new tests in a separate PR?

[bader]

@steffenlarsen, these tests fail in pre-commit, so it would be great to fix them ASAP. Are you okay to commit it "as is" and write new tests in a separate PR?

@steffenlarsen, ping.

[steffenlarsen]

Sorry. Yes, I am okay with this as-is.