[VM] Preserve 64-bit values in internal calls - #24946
Open
zhangpan2001 wants to merge 1 commit into
Open
zhangpan2001 wants to merge 1 commit into
zhangpan2001 wants to merge 1 commit into
Conversation
Internal calls copied one 32-bit register per argument and result, corrupting 64-bit values used as buffer offsets. Use the callee signature to copy values at their full width and align argument registers. Cover consecutive i64 arguments, high result words, and mixed primitive and reference values. Fixes iree-org#24929 Co-authored-by: OpenAI <noreply@openai.com> Signed-off-by: zhangpan <98025960+zhangpan2001@users.noreply.github.com>
zhangpan2001
requested review from
Manewing,
benvanik,
devtbi and
schuermans-roofline
as code owners
September 20, 2026 03:21
schuermans-roofline
left a comment
Contributor
There was a problem hiding this comment.
LGTM. Thanks. However, I have a question about the removed TODO (please see my in-line comment).
| int i32_reg_offset = 0; | ||
| int ref_reg_offset = 0; | ||
| for (int i = 0; i < src_reg_list->size; ++i) { | ||
| // TODO(benvanik): change encoding to avoid this branching. |
Contributor
There was a problem hiding this comment.
This TODO comment had asked for removal of the branching. This PR adds more branching (three cases instead of two), but drops this comment. I do see why introducing the third case makes sense and solves the issue. However, I don't have full understand of what exactly the TODO was suggesting here and why the change solves it. Can you please explain?
(This also applies to the removed "TODO" below.)
This branch has not been deployed
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
2 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.
Internal calls marshaled one 32-bit register per argument and result, so the high word of a 64-bit value was dropped or overwritten by the next argument. When that value feeds a buffer bound it surfaces in HAL command buffer validation as an out-of-range binding, as seen in #24929.
Marshal each argument and result at its full width from the callee's calling convention, and align argument registers to match how external calls and imports are already marshaled.
Adds call_ops.vmasm cases for consecutive i64 arguments, i64 results, and mixed primitive/ref calls. The full VM bytecode test suite passes, and the repro from #24929, which aborted with OUT_OF_RANGE before the fix, now returns the expected output.
Fixes #24929