Sender Side Cache Bypass #1860
Open
Sender Side Cache Bypass #1860
+52
−4
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alex-breslow-amd
requested review from
wenkaidu,
gilbertlee-amd,
PedramAlizadeh,
nusislam,
nileshnegi,
KawtharShafie,
AtlantaPepsi,
mberenjk,
corey-derochie-amd,
mustafabar,
thananon,
JhaShweta1,
BertanDogancay,
rahulvaidya20,
isaki001,
AbandiGa,
Nikhil-Nunna and
haripriya-amd
as code owners
wenkaidu
reviewed
Aug 15, 2025
src/device/op128.h
Outdated
| __builtin_nontemporal_store(value.u64[0], (uint64_t*)addr); \ | ||
| __builtin_nontemporal_store(value.u64[1], (uint64_t*)addr+1); \ | ||
| /*store_bytepack16_##space(addr, value);*/ \ | ||
| asm ("global_store_dwordx4 %0, %1 off sc0 sc1" :: "v"((addr)), "v"((value.u64_vec2))); \ |
There was a problem hiding this comment.
I think need "nt" flag as well to bypass MALL cache
There was a problem hiding this comment.
There appears to be a ~1% performance overhead from setting the nt bit. For multinode, it is significantly more.
alex-breslow-amd
force-pushed
the
cache_bypass
branch
from
ccf9473 to
b079ea2
Compare
alex-breslow-amd
requested review from
PJAvinash,
atulkulk,
ddebonis-amd and
amd-mengshwu
as code owners
|
Bunch of failure on cpx. |
Yup, need to debug. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Do not mention proprietary info or link to internal work items in this PR.
Work item: "Internal", or link to GitHub issue (if applicable).
What were the changes?
Bypass hardware caches on stores and maybe loads.
Why were the changes made?
Seems to be about 4% average uplift for certain GPUs for bfloat16 allreduce for single node. It could be more or less depending on the GPU type.
Cache invalidations and writebacks can be costly. Let's just make this a nonissue. I would like to eventually get us off extended-scope fine-grain memory and just use cache bypassing stores and loads. Then, the mtype of an allocation shouldn't matter. This is important for user-registered buffers where we can't count on the buffers being allocated in extended-scope fine-grain memory. In that case, on sender, we have to execute a system scope release fence. Note, this is currently incorrect in the code in the general case because we use a device scope acquire-release fence. That is only valid when the protocol buffer is marked as extended-scope fine-grain memory.
How was the outcome achieved?
Certain AMD GPUs support hardware cache bypassing loads and stores. I'm exploiting that here.
Additional Documentation:
Seeing some nice performance improvement here for a range of message sizes.
Approval Checklist
Do not approve until these items are satisfied.