Skip to content

Select the transpose vector width from src and dst pointer alignment - #5459

Open
cyyever wants to merge 2 commits into
intel:mainfrom
cyyever:fix/transpose-copy-vector-alignment
Open

cyyever wants to merge 2 commits into
intel:mainfrom
cyyever:fix/transpose-copy-vector-alignment

Conversation

@cyyever

@cyyever cyyever commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

dispatch_transpose picked the vector width purely from rows/cols divisibility, ignoring the actual src/dst addresses. A copy between storage-offset views can have 4-divisible shape yet misaligned addresses, producing misaligned vector loads. Clamp the width with memory::can_vectorize_up_to on both pointers before the divisibility reduction.

Adds a test that does dst.copy_(src) with both src (channels-last) and dst (contiguous) offset by one element (fp16/bf16/fp32).

@github-actions github-actions Bot added disable_e2e Disable all e2e test jobs for the PR disable_distributed Disable distributed UT test jobs for the PR labels Sep 21, 2026
@cyyever
cyyever marked this pull request as draft September 21, 2026 03:21
@cyyever
cyyever marked this pull request as ready for review September 21, 2026 03:22
@cyyever
cyyever force-pushed the fix/transpose-copy-vector-alignment branch from 38d8b0b to 89c7f41 Compare September 21, 2026 03:29
@guangyey

guangyey commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

@torchxpubot review

Comment thread test/xpu/test_transpose_alignment.py Outdated
Comment thread test/xpu/test_transpose_alignment.py Outdated
Co-authored-by: Yu, Guangye <guangye.yu@intel.com>
@torchxpubot

Copy link
Copy Markdown
Contributor

Replying to this comment by @guangyey

No XPU device or torch install is available here (no .venv found), so this is a static review: upstream parity, CI test collection, and the kernel's address arithmetic were all verified by reading source.

PR Review: #5459

Summary

Clamps dispatch_transpose's vector width by the actual src/dst pointer alignment before the rows/cols divisibility reduction, fixing misaligned vector loads/stores when the copy operands are storage-offset views. The kernel change is sound and matches the upstream CUDA approach (maybe_tiled_transpose_copy in aten/src/ATen/native/cuda/Copy.cu gates vectorization on reinterpret_cast<uintptr_t>(sp) % kAlignment == 0 && ... (dp) ...), but the regression test as placed is never executed by CI, so the fix lands with no coverage.

Testing

The test file will never run in CI. test/xpu/test_transpose_alignment.py is not collected by any job:

  • op_ut / skipped_ut run test/xpu/run_test_with_skip.py, which iterates only the ~100 filename keys in test/xpu/skip_list_common.py. The PR does not add the new file there.
  • basic runs op_regression (bare pytest in test/regressions/), op_regression_dev1, and op_extended (test/xpu/extended/) — none touch test/xpu/ root (.github/actions/linux-uttest/action.yml:28-62).
  • torch_xpu does for test in $(ls test/xpu | grep test) from cd pytorch — that's PyTorch's own test/xpu, not this repo's (action.yml:92-101).
  • pull.yml's matrix is [op_ut, basic] plus xpu_distributed.

Move it to test/repro/test_transpose_copy_alignment.py — reproducer-test runs on every PR and pytests changed test/repro/test_*.py (pull.yml:250, _linux_reproducer.yml:61-86) — or to test/regressions/, where bare pytest collects everything and where the sibling copy tests already live (test/regressions/test_copy.py).

The expected value is produced by the code under test. src.copy_(torch.randn(shape, device=device, dtype=dtype)) is itself a contiguous→channels-last dense copy into a 1-element-offset view, so detect_batch_transpose matches and it goes through channels_last_transpose_kernel with a misaligned dst. The assertion assertEqual(dst, src) then compares two tensors both written by the buggy path. Fill src from a CPU tensor and compare dst.cpu() against a CPU-computed reference so the assertion is anchored to ground truth.

No guard that the kernel path was taken. detect_batch_transpose bails out when total_sgs < thread_slots (TransposeKernel.cpp:331-334). For (16, 64, 32, 32) that's 16*32*2 * 8 = 8192 subgroups; on a sufficiently large device the copy silently falls back to copy_kernel and the test passes vacuously. Either assert the path was taken, or size the tensor so the occupancy gate cannot gate it out on supported hardware.

Coverage gaps in the one parametrized case. Only the both-pointers-offset-by-1 case is covered. Missing:

  • Only src misaligned, and only dst misaligned — this is what the std::min over the two pointers exists for, and a one-sided regression would pass today.
  • An offset that exercises the halving loop rather than dropping straight to 1: e.g. fp32 at offset 2 gives can_vectorize_up_to == 2 (16-byte vec4 alignment fails, 8-byte vec2 alignment holds), so vec_size should land on 2, not 1 or 4.

Missing license header. test/xpu/test_transpose_alignment.py starts at # Owner(s): ["module: intel"]. Every file needs the Intel copyright + Apache 2.0 block first; see test/regressions/test_copy.py:1-7.

Use @dtypes, not @parametrize("dtype", ...). The device-generic dtype idiom in this repo is from torch.testing._internal.common_device_type import dtypes (test/regressions/test_copy.py), which produces the conventional ..._xpu_float16 test ids that skip lists and CI reporting key off.

No reproducer evidence in the PR description. The body is a single blockquote with no pytest command line and no before/after output showing the test fails without the kernel change. A pytest line in the description is what preci-lint-check extracts and _linux_reproducer.yml:88-104 re-runs, and it is the only evidence a reviewer has that the test actually reproduces the bug.

Testing (minor)

offset_view(offset, view_shape) sizes its allocation from the enclosing shape rather than from its own view_shape parameter. It works only because both shapes happen to have the same numel; use math.prod(view_shape).

Recommendation

Request Changes

The kernel fix is correct — I verified the access offsets (batch*rows*cols + src_row*cols + src_col for loads, + dst_row*rows + dst_col for stores) are VEC_SIZE-aligned relative to the base pointer exactly when cols % VEC_SIZE == 0 and rows % VEC_SIZE == 0, so base-pointer alignment was the missing condition, and can_vectorize_up_to keys off alignof(aligned_vector<scalar_t, N>), which is the same alignment the kernel's vec_t reinterpret_cast requires. What blocks approval is the test: it sits in a directory no CI job collects, so the bug fix ships uncovered, and its expected value is generated by the buggy path. Move the test to test/repro/ or test/regressions/, anchor it to a CPU reference, add the one-sided-misalignment and halving-loop cases, and add the license header.


Generated by review job.

@guangyey guangyey left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you please help move the test to test/regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

disable_distributed Disable distributed UT test jobs for the PR disable_e2e Disable all e2e test jobs for the PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants