[SYCL][E2E] Avoid illegal failure order in compare_exchange_strong test #19513
Conversation
The AtomicRef/compare_exchange_strong E2E test allowed `acq_rel` and `release` being passed as the failure memory order. This is not allowed by the SYCL AtomicRef spec (see Table 132), and might cause failures on some backends. This change explicitly sets the failure order argument in compare_exchange_strong, instead of relaying on an overload function, which used the same order for failure and success.
| // operation must be relaxed, acquire or seq_cst. | ||
| auto failure_order = | ||
| (order == memory_order::acq_rel || order == memory_order::release) | ||
| ? memory_order::relaxed |
There was a problem hiding this comment.
Why don't we just assert that failure_order is allowed and then modify the callers? Is that because we use the same parameter to test multiple functions and other functions can accept that?
If so, I think we should use the strictest memory order and not the weakest instead of the unsupported, because theoretically the caller might be checking for side effects that weaker one can't guarantee.
There was a problem hiding this comment.
I've chosen memory_order::relaxed as the fallback for failure_order, because that is what is used for the default memory order fallback for AtomicRef in this test. I can change this to acquire -- I don't think anything stronger is required since there are no side effects to release on failure.
Another possibility, and my preferred route, is changing the sycl::AtomicRef::compare_exchange_strong() function to change illegal failure memory orders to the next best legal ones: use acquire instead of acq_rel and relaxed instead of release. This would match the behaviour of the C++ std library: https://en.cppreference.com/w/cpp/atomic/atomic_ref/compare_exchange
Re modifying the test callers. The compare_exchange_test_orders_scopes template function generates all the memory orders to test. This order template parameter is ultimately used for both success_order and failure_order in compare_exchange_strong. If we prevent calls with acq_rel and release being used for order in compare_exchange_test_orders_scopes, then success_order will also not be tested with these variants. If we care about all combinations of success_order and failure_order being tested, then we could add another template function that generates all allowed failure orders, in addition to all possible success orders and memory scopes. However, I don't think the E2E are expected to test every possible parameter combination.
There was a problem hiding this comment.
+ @gmlueck for
Another possibility, and my preferred route, is changing the sycl::AtomicRef::compare_exchange_strong() function to change illegal failure memory orders to the next best legal ones: use acquire instead of acq_rel and relaxed instead of release. This would match the behaviour of the C++ std library: https://en.cppreference.com/w/cpp/atomic/atomic_ref/compare_exchange
The AtomicRef/compare_exchange_strong E2E test allowed
acq_relandreleasebeing passed as the failure memory order. This is not allowed by the SYCL AtomicRef spec (see Table 132), and might cause failures on some backends.This change explicitly sets the failure memory order argument in compare_exchange_strong to avoid
acq_relandreleasebeing passed. Previously, an overloaded compare_exchange_strong function was used, which set the same memory order for failure and success.