Skip to content

Fix alignment for in-memory allreduce buffer#12082

Open
RAMitchell wants to merge 2 commits intodmlc:masterfrom
RAMitchell:fix-cpu-reinterpret-alignment
Open

Fix alignment for in-memory allreduce buffer#12082
RAMitchell wants to merge 2 commits intodmlc:masterfrom
RAMitchell:fix-cpu-reinterpret-alignment

Conversation

@RAMitchell
Copy link
Member

Summary

Fix alignment for the shared buffer used by in-memory collective allreduce.

The previous implementation stored bytes in a std::string and then reinterpreted the buffer as typed data for reduction. That provides contiguous storage, but not the alignment guarantees required for typed access. This change switches the shared buffer to aligned storage while preserving the existing byte-oriented behavior for the in-memory collective path.

No intended user-facing changes.

Verification

Built src/collective/in_memory_handler.cc in the existing build/ tree.

@RAMitchell RAMitchell requested a review from Copilot March 13, 2026 10:25
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Replaces std::string with a custom AlignedByteBuffer backed by std::max_align_t storage to guarantee proper alignment for typed reinterpret_casts during in-memory allreduce.

Changes:

  • Introduced AlignedByteBuffer class providing aligned byte storage with a std::string-like interface
  • Updated all functor signatures and buffer operations to use AlignedByteBuffer
  • Added alignment/size checks in the allreduce path

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/collective/in_memory_handler.h Added AlignedByteBuffer class; changed buffer_ member type
src/collective/in_memory_handler.cc Updated functors to use AlignedByteBuffer; added checks; fixed Accumulate signature

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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