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

[HIP] Revert add prefetch for USM hip allocations a6b8fa66b537753415d24076f… #936

Merged
merged 1 commit into from
Nov 24, 2023

Conversation

hdelan
Copy link
Contributor

@hdelan hdelan commented Oct 9, 2023

intel/llvm#10430 caused a performance regression. The new way to ensure safety when using xor atomics is by changing the xor impl to use a CAS loop. See here intel/llvm#11876.

Tested in intel/llvm#11893.

@hdelan hdelan changed the title Revert add prefetch for USM hip allocations a6b8fa66b537753415d24076f… [HIP] Revert add prefetch for USM hip allocations a6b8fa66b537753415d24076f… Oct 9, 2023
@kbenzie kbenzie requested a review from a team October 20, 2023 13:25
@hdelan hdelan requested a review from jchlanda October 24, 2023 17:18
@jchlanda
Copy link
Contributor

Has this been done through git revert? If so why do we need 2 commits?

@hdelan hdelan force-pushed the revert-hip-prefetch branch from 77845ea to 7de531a Compare October 25, 2023 09:26
@hdelan
Copy link
Contributor Author

hdelan commented Oct 25, 2023

Has this been done through git revert? If so why do we need 2 commits?

Yes it was but I think maybe there was some conflict so added the second commit. Have squashed the second commit so just one commit now

@hdelan hdelan force-pushed the revert-hip-prefetch branch from 7de531a to b3887e6 Compare October 25, 2023 09:28
Copy link
Contributor

@jchlanda jchlanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

steffenlarsen pushed a commit to intel/llvm that referenced this pull request Nov 15, 2023
…f atomic builtin (#11876)

1. Fix a race condition in cmpxchg
2. Change `atomic_xor` to use a CAS loop instead of atomic builtin. This
is needed to merge this in UR
oneapi-src/unified-runtime#936 so that perf
regression can be fixed. The long term fix is to use a compiler flag to
choose between builtin and safe CAS implementation, but talks upstream
may take some time to figure out the details. See
llvm/llvm-project#69229
@kbenzie
Copy link
Contributor

kbenzie commented Nov 15, 2023

This needs an intel/llvm testing PR with passing checks before we can merge here.

@hdelan
Copy link
Contributor Author

hdelan commented Nov 15, 2023

intel/llvm#11893

@hdelan hdelan force-pushed the revert-hip-prefetch branch from b3887e6 to 1151231 Compare November 20, 2023 11:33
@hdelan hdelan added the ready to merge Added to PR's which are ready to merge label Nov 21, 2023
@hdelan hdelan force-pushed the revert-hip-prefetch branch from 1151231 to 841a287 Compare November 21, 2023 15:08
@hdelan hdelan mentioned this pull request Nov 21, 2023
@kbenzie kbenzie merged commit 28cf40f into oneapi-src:adapters Nov 24, 2023
48 checks passed
steffenlarsen pushed a commit to intel/llvm that referenced this pull request Nov 28, 2023
Run CI for oneapi-src/unified-runtime#936

Depends on #11718 merging first.

---------

Co-authored-by: Kenneth Benzie (Benie) <k.benzie@codeplay.com>
callumfare pushed a commit to kbenzie/intel-llvm that referenced this pull request Dec 18, 2023
Run CI for oneapi-src/unified-runtime#936

Depends on intel#11718 merging first.

---------

Co-authored-by: Kenneth Benzie (Benie) <k.benzie@codeplay.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Added to PR's which are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants