Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use __hip_atomic_fetch_sub #7

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ldrumm
Copy link

@ldrumm ldrumm commented Aug 24, 2023

Where available, __hip_atomic_fetch_sub can be used to implement the atomicSub family.

Introduced in llvm e3fbede7f3f

Where available, `__hip_atomic_fetch_sub` can be used to implement the
`atomicSub` family.

Introduced in llvm e3fbede7f3f
@cjatin
Copy link
Contributor

cjatin commented Sep 4, 2023

@yxsamliu should we use the fetch_sub?

@ldrumm
Copy link
Author

ldrumm commented Sep 20, 2023

Thanks for checking on this, Jatin.

I've just found out that this isn't practically feasible at the moment. Atomic sub operations on shared USM address ranges can be improperly handled by the PCIe bus without explicit prefetches, and given that the Rocm/HSA drivers have no way to catch this, it can result in these sub operations not happening (see intel/llvm#7252 and this rocm ticket for details). However, I think that behaviour is a bug that needs to be addressed, and that correctness must be present at the device and driver level before anything on top can work. I'll leave this patch open as a reminder, but understand that merging it will break HIP atomics a little more until the lower levels of the stack work reliably

@yxsamliu
Copy link
Contributor

probably we should not use __hip_atomic_fetch_sub for atomicSub_system since it does not work across PCIE. We may still use it for atomicSub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants