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

Support Simple protocol plans using scratch buffer #371

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

yzygitzh
Copy link
Member

@yzygitzh yzygitzh commented Oct 22, 2024

  1. Separate the options whether to use packet and whether to use double buffer.
  2. Reduce primitive implementation fixes in executor.
  3. Add validation for number of operations, channels and channels per operation.

@yzygitzh yzygitzh changed the title Add separate option for double scratch buffer Support Simple protocol plans using scratch buffer Oct 24, 2024
src/executor/executor.cc Show resolved Hide resolved
Comment on lines 90 to 94
plan.impl_->lightLoadExecutionPlan(inputMessageSize, outputMessageSize, contsSrcOffset, constDstOffset, rank,
sendBufferSize, recvBufferSize, flag);
this->setupDeviceExecutionPlan(this->contexts[key], rank, plan);
this->contexts[key].deviceExecutionPlansBuffer =
allocExtSharedCuda<char>(this->contexts[key].deviceExecutionPlans.size() * sizeof(DeviceExecutionPlan));
Copy link
Contributor

Choose a reason for hiding this comment

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

If find the key, seems it will overwrite the previous plan. It may cause problem when using cuda graph. Only last plan will be copied to GPU memory. Maybe we need to distinguish the plan with inputMessageSize, outputMessageSize, flag. And let kernel load the plan with correct device_ptr.

@chhwang
Copy link
Contributor

chhwang commented Dec 18, 2024

@Binyang2014 Any updates in this PR?

@Binyang2014
Copy link
Contributor

@Binyang2014 Any updates in this PR?

I think there not blocker for this PR. Just need to resolve the conflict. Let me check with Ziyue

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.

3 participants