hipfile/fastpath: Ensure (buffer_address + buffer_offset) is aligned#175
Open
kurtmcmillan wants to merge 12 commits intodevelopfrom
Open
hipfile/fastpath: Ensure (buffer_address + buffer_offset) is aligned#175kurtmcmillan wants to merge 12 commits intodevelopfrom
kurtmcmillan wants to merge 12 commits intodevelopfrom
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes alignment validation in the fastpath implementation by ensuring that the combined buffer_address + buffer_offset is properly aligned, rather than checking only buffer_offset in isolation.
Changes:
- Updated alignment checks to validate
buffer_address + buffer_offsetinstead of justbuffer_offset - Added comprehensive test coverage including a new
TestDefaultstest andScoreRejectsBufferAddressPlusBufferOffsetIsUnalignedtest - Expanded parameterized test cases with additional edge cases and conditional compilation guards
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/amd_detail/backend/fastpath.cpp | Modified alignment validation logic to check combined buffer address and offset |
| test/amd_detail/fastpath.cpp | Added new tests, updated default test values for proper alignment, and added EXPECT_CALL for getBuffer() across all tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kurtmcmillan
force-pushed
the
kumcmill/fix-fastpath-mem-align-check
branch
from
1250c5b to
288426a
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kurtmcmillan
force-pushed
the
kumcmill/fix-fastpath-mem-align-check
branch
from
288426a to
2904af6
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kurtmcmillan
force-pushed
the
kumcmill/fix-fastpath-mem-align-check
branch
from
2904af6 to
3b23b45
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kurtmcmillan
requested review from
derobins,
gaoikawa,
jordan-turbofish and
riley-dixon
as code owners
jordan-turbofish
approved these changes
Jan 30, 2026
- Ensure DEFAULT_MEM_ALIGN and DEFAULT_OFFSET_ALIGN are powers of 2 (> 1) - Ensure DEFAULT_BUFFER_ADDR is DEFAULT_MEM_ALIGN aligned - Ensure DEFAULT_BUFFER_OFFSET is DEFAULT_MEM_ALIGN aligned - Ensure DEFAULT_FILE_OFFSET is DEFAULT_OFFSET_ALIGN aligned - Ensure DEFAULT_IO_SIZE is DEFAULT_OFFSET_ALIGN aligned
FastpathAlignedIoSizesParam should only be instantiated with size_t values. -DEFAULT_OFFSET_ALIGN became 4294966784 in the test.
FastpathUnalignedIoSizesParam should only be instantiated with size_t values. (-DEFAULT_OFFSET_ALIGN + 1) became 4294966785 in the test.
Cast DEFAULT_OFFSET_ALIGN to hoff_t before negating to ensure the test is
instantiated with the expected value.
-DEFAULT_OFFSET_ALIGN -> 4294966784
-static_cast<hoff_t>(DEFAULT_OFFSET_ALIGN) -> -512
…Param
Cast DEFAULT_OFFSET_ALIGN to hoff_t before negating to ensure the test is
instantiated with the expected value.
-DEFAULT_OFFSET_ALIGN -> 4294966784
-static_cast<hoff_t>(DEFAULT_OFFSET_ALIGN) -> -512
…fsetsParam We should be passing in a file offset value not a buffer offset value.
…Param
Cast DEFAULT_OFFSET_ALIGN to hoff_t before negating to ensure the test is
instantiated with the expected value.
-DEFAULT_MEM_ALIGN -> 4294967292
-static_cast<hoff_t>(DEFAULT_MEM_ALIGN) -> -4
…tsParam
Cast DEFAULT_OFFSET_ALIGN to hoff_t before negating to ensure the test is
instantiated with the expected value.
-DEFAULT_MEM_ALIGN -> 4294967292
-static_cast<hoff_t>(DEFAULT_MEM_ALIGN) -> -4
Removes alignment check duplication.
kurtmcmillan
force-pushed
the
kumcmill/fix-fastpath-mem-align-check
branch
from
3b23b45 to
af88d42
Compare
riley-dixon
approved these changes
Feb 4, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
No description provided.