fix: unaligned `memcpy` by greenhat · Pull Request #1004 · 0xMiden/compiler

greenhat · 2026-03-12T13:08:35Z

Close #1003

Summary

factor the counted loop setup used by memcpy/memset and add regression coverage for aligned and unaligned byte copies/sets, zero-length operations, and unaligned u16/i16 memory accesses
tighten MASM memory emission for byte-oriented memcpy fast paths so they only convert byte pointers to element addresses when the inputs are word-aligned
handle unaligned 16-bit loads and stores that cross a 32-bit element boundary by routing offset == 3 through the split-word intrinsics while preserving the existing within-element path for offset <= 2

I suggest reviewing on a per-commit basis skipping non-interesting commits (refactors, etc.).

bitwalker

I think this mostly looks fine, I left comments for my various nits, but nothing major sticks out.

bitwalker · 2026-03-23T01:08:34Z

codegen/masm/intrinsics/mem.masm

+# which is the offset of the first byte, in the 32-bit representation of that element.
+#
+# Stack transition: [addr, offset] -> [value]
+pub proc load_u16


Suggested change

pub proc load_u16

pub proc load_u16(addr: ptr<felt, addrspace(felt)>, offset: u8) -> u16

Let's make sure to add type signatures to all new procedures

Thanks! Done in c180217

bitwalker · 2026-03-23T01:10:35Z

codegen/masm/src/emit/mem.rs

 impl OpEmitter<'_> {
+    /// Emit the loop header for a counted `while.true` loop.
+    ///
+    /// The caller provides the `dup` instruction needed to bring `count` to the top of the stack


I think it would help to clarify a bit why this is up to the caller, and perhaps provide an example. IMO, it would make more sense to provide an offset value or something, rather than the instruction itself - but the rationale might be obvious from better documentation/examples here.

Done in c180217

bitwalker · 2026-03-23T01:10:57Z

codegen/masm/src/emit/mem.rs

+
+    /// Emit the loop back-edge condition for a counted `while.true` loop.
+    ///
+    /// The caller provides the `dup` instruction needed to bring `count` to the top of the stack


Done in c180217

bitwalker · 2026-03-23T01:12:02Z

codegen/masm/src/emit/mem.rs

+        self.emit_all(
+            [
+                masm::Instruction::U32DivModImm(16.into()),
+                masm::Instruction::Assertz,


We should probably consider using an instruction variant with an error message, so it is clear why this fails when it fails

Awesome! Thank you! Done in c180217

bitwalker · 2026-03-23T01:14:00Z

codegen/masm/src/emit/mem.rs

+    ///
+    /// This is used for branch bodies which operate on a known stack shape from the enclosing
+    /// emitter, but which do not need to synchronize typed operand-stack state back to it.
+    fn build_raw_block(


Is there a distinction between a "raw" MASM block and a non-"raw" MASM block? I think I might just call this build_masm_block or something

Done in c180217

bitwalker · 2026-03-23T01:14:56Z

codegen/masm/src/emit/mem.rs

+                        [
+                            // Convert `src` to element address
+                            masm::Instruction::U32DivModImm(4.into()),
+                            masm::Instruction::Assertz,


We should probably try and make these asserts produce useful errors

Done in c180217

I also went and added error messages to all emitted asserts at 16a7669

greenhat · 2026-03-25T15:32:43Z

Please don't merge this PR before #995. It's a pain to rebase it afterwards.

bitwalker · 2026-03-26T17:19:10Z

This just needs a rebase and it can be merged

Fix the byte-addressed memory paths that cross a 32-bit element boundary. This keeps the `memcpy`/`memset` fallback coverage added in this branch working for short unaligned copies, including scalarized `u16` loads and stores at byte offset 3.

Zero-length memory operations must be no-ops, but both loop headers seeded `while.true` with `count >= 0`, which executes one iteration when `count == 0`. Switch the entry condition to a strict unsigned `count > 0` check and add regressions for zero-count unaligned copy/set paths.

The unaligned `u16` regressions are asserting compiler memory layout, so they should not depend on the host endianness. Use `to_le_bytes()` in the expected byte construction to keep the tests portable and aligned with the byte-addressable memory model.

`memset` and fallback `memcpy` were carrying separate copies of the same counted `while.true` control flow, which makes fixes easy to miss in one path. Extract the shared loop header and back-edge emission so the counted loop protocol is defined once and reused by both sites.

Only offset 3 spans two elements for a `u16` load/store. Route the other unaligned offsets through the existing single-element logic so we don't spuriously touch `addr + 1` at the end of memory.

Add regression cases for byte offsets 1 and 2 in the integration suite, and add emitter-level tests that exercise unaligned `load_imm` and `store_imm` for `u16` addresses.

Cover the aligned byte-copy fast path and a case where only `count` is misaligned so the fast-path predicate is regression-tested as well.

greenhat changed the title ~~fix: byte-version of the memcpy (iteration guard)~~ fix: byte-version of the unaligned memcpy Mar 12, 2026

greenhat force-pushed the i1003-fix-byte-memcpy branch from 3f5b5d0 to 30b783a Compare March 17, 2026 05:02

greenhat marked this pull request as ready for review March 18, 2026 14:03

greenhat changed the title ~~fix: byte-version of the unaligned memcpy~~ fix: unaligned memcpy Mar 18, 2026

greenhat requested a review from bitwalker March 18, 2026 14:04

bitwalker approved these changes Mar 23, 2026

View reviewed changes

greenhat added 22 commits March 27, 2026 09:54

fix: byte-version of the memcpy (iteration guard)

d4707c3

fix: handle unaligned u16 memory windows

f137a87

Fix the byte-addressed memory paths that cross a 32-bit element boundary. This keeps the `memcpy`/`memset` fallback coverage added in this branch working for short unaligned copies, including scalarized `u16` loads and stores at byte offset 3.

fix: limit cross-element u16 memory access

e3588f9

Only offset 3 spans two elements for a `u16` load/store. Route the other unaligned offsets through the existing single-element logic so we don't spuriously touch `addr + 1` at the end of memory.

test: cover unaligned u16 offsets and immediates

744b707

Add regression cases for byte offsets 1 and 2 in the integration suite, and add emitter-level tests that exercise unaligned `load_imm` and `store_imm` for `u16` addresses.

test: add aligned byte memcpy coverage

125fa5c

Cover the aligned byte-copy fast path and a case where only `count` is misaligned so the fast-path predicate is regression-tested as well.

test: strengthen unaligned u16 emitter assertions

54b4f74

test: cover signed unaligned i16 memory access

8c82689

refactor: share split-element u16 branch emission

83b71fa

docs: complete memcpy semantics comment

da32974

fix: assert word alignment in memcpy word fast paths

35c1cd5

Clarify 16-bit memcpy helper naming

efbc276

Clean up memcpy word-copy fast path flow

9ca4499

Deduplicate unaligned 16-bit intrinsic tests

6639266

Cover memcpy word-copy fast paths

1826cee

Document cross-element 16-bit store window

5a786e8

Restrict memcpy word fast paths to byte pointers

c69fb7d

Introduce raw MASM branch block builder

eac758a

Rename aligned byte memcpy regression

a0ef2e6

Add signed 16-bit immediate path tests

9362cfe

greenhat added 7 commits March 27, 2026 09:54

Refactor u16 memory access lowering

a38a75b

Add constant-address u16 load coverage

7369401

Remove redundant u16 load tests

93f42b7

chore: address PR 1004 review notes

17b7e4a

chore: add error messages to all emitted Instruction::Assert* ops

2aa50e4

fix: build after the rebase

c803c42

fix: migrate the #1003 fix to the VM v0.22

4a529fd

greenhat force-pushed the i1003-fix-byte-memcpy branch from 16a7669 to 4a529fd Compare March 27, 2026 09:13

greenhat merged commit f7578b1 into next Mar 27, 2026
15 checks passed

greenhat deleted the i1003-fix-byte-memcpy branch March 27, 2026 09:42

	pub proc load_u16
	pub proc load_u16(addr: ptr<felt, addrspace(felt)>, offset: u8) -> u16

Conversation

greenhat commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

bitwalker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greenhat commented Mar 25, 2026

Uh oh!

bitwalker commented Mar 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

greenhat commented Mar 12, 2026 •

edited

Loading