Skip to content

In-place ops and caller-provided destinations - #10

Closed
AntonioPalese wants to merge 1 commit into
mainfrom
P2
Closed

AntonioPalese wants to merge 1 commit into
mainfrom
P2

Conversation

@AntonioPalese

Copy link
Copy Markdown
Owner

Every Tensor method was const and allocated its result: one allocation and
one free per op per iteration in a training loop, and a peak footprint equal
to the whole expression graph rather than the working set.

Each op now has three forms, of which exactly one is a real implementation:

auto c = a.add(b, s);      // allocates the result, then calls add_out
a.add_out(b, out, s);      // <- the body
a.add_(b, s);              // == a.add_out(b, a, s)

add_out returns Tensor& (the destination), add_ returns *this, so calls
chain; the no-stream spellings are one-line delegates. operator+= -= *= /=
delegate to the underscore family.

Families: add/sub/mul/div (tensor and scalar), apply, apply_binary, relu,
sigmoid and fill_ have all three forms. matmul, transpose and permute have
_out but no in-place form -- their kernels read elements they do not write,
so _check_alias_none rejects an aliased destination rather than returning a
plausible wrong answer.

The elementwise family may alias because the CPU loop, the contiguous GPU
fast path and the rank-specialized kernels all read index i and write index
i. That rests on each buffer being one flat run, so _check_alias_elementwise
re-checks is_contiguous(): the day a strided view can alias a different
region of the same allocation, it throws instead of computing the wrong
thing.

Consequences:

  • contiguous.cuh drops restrict from its three kernels. In-place calls
    them with dst equal to a source, which is precisely the aliasing the
    qualifier promises away. Measured, it cost nothing: add at 16 M went
    230.2-233.4 -> 228.6-230.7 GB/s and relu 232.9-234.6 -> 233.4-235.5, i.e.
    inside run-to-run spread. The read-only path comes from the explicit __ldg
    in device_load, not from restrict-driven inference.
  • _out is where operand validation now lives, so a GPU add between
    mismatched shapes throws instead of running off the end of a buffer.
  • fill_ takes a stream and no longer routes through the legacy _fill
    dispatch struct, which now has no callers at all.
  • apply_binary gained the stream overload roadmap 4.4 was asking for, since
    apply_binary_out takes one by construction.

Numbers (benchmark_report.md section 9, harness scripts/bench_inplace.py):
1.2-1.6x below ~64 K on both backends where allocation is a large fraction
of the op, shrinking to 1.04x at 16 M where the kernel dominates and
HostPool/cudaMallocAsync already recycle the block. That is the honest
ceiling for the time argument. The memory argument does not shrink, and the
tests assert it: data_ptr unchanged across a 100-step loop, the property a
correct-but-reallocating implementation would fail while passing every value
check.

Python gets the whole family through int-returning C entry points (nothing
is allocated, no ownership changes hands), with iadd and friends
returning self so a += b does not silently rebind the name.

tests/test_inplace.cpp (25 tests) and python/tests/test_inplace.py (35).
All 13 correctness suites and 174 Python tests pass; compute-sanitizer
memcheck clean over test_inplace, test_streams, test_allocator_stream and
test_stress.

Co-Authored-By: Claude Opus 5 noreply@anthropic.com

Every Tensor method was const and allocated its result: one allocation and
one free per op per iteration in a training loop, and a peak footprint equal
to the whole expression graph rather than the working set.

Each op now has three forms, of which exactly one is a real implementation:

    auto c = a.add(b, s);      // allocates the result, then calls add_out
    a.add_out(b, out, s);      // <- the body
    a.add_(b, s);              // == a.add_out(b, a, s)

add_out returns Tensor& (the destination), add_ returns *this, so calls
chain; the no-stream spellings are one-line delegates. operator+= -= *= /=
delegate to the underscore family.

Families: add/sub/mul/div (tensor and scalar), apply, apply_binary, relu,
sigmoid and fill_ have all three forms. matmul, transpose and permute have
_out but no in-place form -- their kernels read elements they do not write,
so _check_alias_none rejects an aliased destination rather than returning a
plausible wrong answer.

The elementwise family may alias because the CPU loop, the contiguous GPU
fast path and the rank-specialized kernels all read index i and write index
i. That rests on each buffer being one flat run, so _check_alias_elementwise
re-checks is_contiguous(): the day a strided view can alias a different
region of the same allocation, it throws instead of computing the wrong
thing.

Consequences:

- contiguous.cuh drops __restrict__ from its three kernels. In-place calls
  them with dst equal to a source, which is precisely the aliasing the
  qualifier promises away. Measured, it cost nothing: add at 16 M went
  230.2-233.4 -> 228.6-230.7 GB/s and relu 232.9-234.6 -> 233.4-235.5, i.e.
  inside run-to-run spread. The read-only path comes from the explicit __ldg
  in device_load, not from restrict-driven inference.
- _out is where operand validation now lives, so a GPU add between
  mismatched shapes throws instead of running off the end of a buffer.
- fill_ takes a stream and no longer routes through the legacy _fill
  dispatch struct, which now has no callers at all.
- apply_binary gained the stream overload roadmap 4.4 was asking for, since
  apply_binary_out takes one by construction.

Numbers (benchmark_report.md section 9, harness scripts/bench_inplace.py):
1.2-1.6x below ~64 K on both backends where allocation is a large fraction
of the op, shrinking to 1.04x at 16 M where the kernel dominates and
HostPool/cudaMallocAsync already recycle the block. That is the honest
ceiling for the time argument. The memory argument does not shrink, and the
tests assert it: data_ptr unchanged across a 100-step loop, the property a
correct-but-reallocating implementation would fail while passing every value
check.

Python gets the whole family through int-returning C entry points (nothing
is allocated, no ownership changes hands), with __iadd__ and friends
returning self so `a += b` does not silently rebind the name.

tests/test_inplace.cpp (25 tests) and python/tests/test_inplace.py (35).
All 13 correctness suites and 174 Python tests pass; compute-sanitizer
memcheck clean over test_inplace, test_streams, test_allocator_stream and
test_stress.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b20bf6a17b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread headers/tensor.inl
Comment on lines +531 to +532
if (out.m_Data == nullptr)
throw std::invalid_argument(std::string(who) + ": destination has been moved from");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Allow zero-sized destinations

nullptr is a valid allocation result for empty tensors: both CPU and GPU allocators return it when the element count is zero. This check therefore makes every refactored operation fail for valid empty shapes—for example, Tensor<float> a({0}), b({0}); a.add(b) allocates an empty out and then immediately throws here. Distinguish a moved-from tensor from a valid zero-element tensor instead of using m_Data == nullptr alone.

Useful? React with 👍 / 👎.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant