Skip to content

Fix pad with an axes subset and negative axes - #4364

Draft
kapellirohith wants to merge 1 commit into
ml-explore:mainfrom
kapellirohith:edge-pad-axes
Draft

Fix pad with an axes subset and negative axes#4364
kapellirohith wants to merge 1 commit into
ml-explore:mainfrom
kapellirohith:edge-pad-axes

Conversation

@kapellirohith

@kapellirohith kapellirohith commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Proposed changes

edge_pad ignores its axes argument and indexes the pad-size vectors by
array axis rather than by position in axes (ops.cpp:1550, 1556). Padding a
subset of the axes therefore reads past the end of those vectors, and a
permuted axes list silently pads the wrong axis:

auto x = reshape(arange(25.0f), {5, 5});
pad(x, {1, 0}, Shape{1, 2}, Shape{1, 2}, array(0.0f), "edge");
// input lands on the wrong axis and 3 rows are left zero; no error

A debug build traps in SmallVector::operator[] with edge_pad at
ops.cpp:1551 on the stack. ASan is quiet at small ndim because Shape is a
SmallVector with 10 inline slots, so the read stays inside the object. Once
ndim puts the pad sizes on the heap it reports the overflow directly:

ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 4 ... 0 bytes after 128-byte region
    #0 mlx::core::edge_pad(...)

reflect_pad indexes by position and is correct for a subset, but it does not
normalize a negative axis (ops.cpp:1468, 1476), so axes = {-1} indexes
starts[-1]:

small_vector.h:316: runtime error: addition of unsigned offset to
0x...060 overflowed to 0x...058

Axis handling across the pad family now goes through normalize_axis_index,
the same consolidation #4288 applied to split, unstack, partition and
topk. That helper also range checks, which changes behaviour in one place:
pad previously indexed out_shape with an unvalidated axis (ops.cpp:1621),
so an out-of-range axis was undefined behaviour for every mode. It now raises
std::invalid_argument. That is a new error path, not just a bug fix.

pad validates and normalizes every axis before the mode dispatch, so the
calls inside edge_pad and reflect_pad are redundant by construction.
Normalizing once in pad and passing the normalized vector down measures
+27/-20 on this file against +22/-18, and it changes what the Pad primitive
stores, and with it is_equivalent, state() and the axes Pad::vmap sees.
Keeping the helper at each site leaves the primitive untouched, but the
single-call version is a small change if you prefer it.

edge_pad landed in 635ccd9 (2024-08-06, #1309). reflect_pad is newer,
9f35f77 (2026-08-11, #3608), and already indexes by position; this adds the
axis normalization it was missing and brings edge_pad in line with it.
constant was already correct in both respects.

This is reachable from mlx_pad in mlx-c (mlx/c/ops.h:730), which forwards
caller-supplied axes, independent pad-size lengths, and a mode string. No
Python path reaches it: the bindings only build full-length axes
(python/src/ops.cpp:3493), which is also why the existing tests miss it, the
only C++ test on this overload using a 1-D array where axes.size() == ndim.

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING document
  • I have run pre-commit run --all-files to format my code / installed pre-commit prior to committing changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the necessary documentation (if needed)

edge_pad ignored its axes argument and indexed the pad-size vectors by
array axis, reading past their end when padding a subset of the axes and
placing the input on the wrong axis when axes was permuted. reflect_pad
indexed by position but never normalized a negative axis.

Axis handling in the pad family now goes through normalize_axis_index,
which also range checks, replacing the hand-rolled normalization in
pad() that indexed out_shape with an unvalidated axis.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants