[logic-rewrite-hip] Fix memory ordering bug causing HSA_STATUS_ERROR_MEMORY_FAULT on MI300X

[maarquitos14]

The benchmark was crashing with HSA_STATUS_ERROR_MEMORY_FAULT on MI300X (gfx942). However, it was running fine on MI250X (gfx90a). According to my investigation, the problem is a missing memory ordering guarantee in the linked-list hash table used by the ReplaceSubgr kernel.

More specifically, TableInsert writes a .val using a plain store, and then publishes a pointer to that entry using atomicCAS on .next. TableLookup follows the .next chain, reads .val, and uses it as an index for some global arrays (e.g. fanin0, fanin1). On AMD GPUs, atomicCAS has relaxed memory ordering, which means that it guarantees atomicity of the CAS'd word, but does not order surrounding plain stores. This works for gfx90a because a single kernel runs on a single GCD with a shared L2 cache, so .val store likely propagates quickly enough that no thread ever observes a wrong value. However, on gfx942, a single kernel can run on up to 8 XCDs, each of them using their own L2. Under this scenario, a thread on one XCD can follow a valid .next pointer, while .val update wasn't propagated yet. The thread reads an unititialized .val and uses it as an array index, causing the memory fault.

To confirm the hypothesis above I added some instrumentation and extra checks to the kernel, and found that .val = -1 was read hundreds of times, and later used as an index to the arrays mentioned above. After applying the fix, this never happened in 20+ runs.

The fix replaces the relaxed atomicCAS in TableInsert with a release-CAS (i.e. explicitly use __ATOMIC_RELEASE as order), and replaces plain load of .next in TableLookup with acquire-loads (i.e. explicit use of __ATOMIC_ACQUIRE as order). Both use __HIP_MEMORY_SCOPE_AGENT as scope, the narrowest scope that covers cross-XCD communication. The release on the CAS ensures all prior stores (including .val) are globally visible before .next is published. The acquire on the load ensures the reader sees those stores after following .next.

__threadfence() was also considered as an alternative, but the release-acquire approach handles this case more efficiently.

[zjin-lcf]

Thank you for the analysis and explanation. I browsed the changes. There are some HIP intrinsic functions I am not familiar with. Can the high-level APIs (e.g. atomicCAS) address the issue ?

[maarquitos14]

Thank you for the analysis and explanation. I browsed the changes. There are some HIP intrinsic functions I am not familiar with. Can the high-level APIs (e.g. atomicCAS) address the issue ?

As far as I know, there is no way to express order and scope in the high-level API: https://rocm.docs.amd.com/projects/HIP/en/latest/how-to/hip_cpp_language_extensions.html#atomic-functions