[RNE Rewrite] Use std::span instead of raw pointers in native C++ ops + tokenizer cosmetic refactor#1315
Open
msluszniak wants to merge 1 commit into
Open
[RNE Rewrite] Use std::span instead of raw pointers in native C++ ops + tokenizer cosmetic refactor#1315msluszniak wants to merge 1 commit into
std::span instead of raw pointers in native C++ ops + tokenizer cosmetic refactor#1315msluszniak wants to merge 1 commit into
Conversation
Applies the treatment from the FSMN-VAD framing op (#1298) to the remaining native ops that indexed tensor buffers through raw pointers: wrap the buffer in a span, take a subspan per row/lane, and use range-based algorithms instead of manual pointer arithmetic. - math: sigmoid/threshold become std::ranges::transform; softmax/argmax take a per-lane subspan instead of re-deriving offsets from a pointer. - cv/image_ops: plane and pixel access go through subspans; the toChannelsFirst memcpy becomes std::ranges::copy. - cv/box_ops: decodeToXyxy now takes a std::span<const float, 4> row rather than four loose floats unpacked at each call site. Byte-level spans are bounded by tensor size_ (bytes) and element spans by numel_, so every subspan is bounds-checked against the real allocation. Also drops the five repeated `if (!result.ok())` blocks in the tokenizer in favour of a local `unwrap` helper, mirroring the one in core/model.cpp (that one is file-local and typed to executorch::runtime::Result, so it cannot be reused for tokenizers::Result). Closes #1308
std::span instead of raw pointers in native C++ opsstd::span instead of raw pointers in native C++ ops + tokenizer cosmetic refactor
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
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.
Description
Applies the treatment from the FSMN-VAD framing op (#1298) to the remaining native ops that indexed tensor buffers through raw pointers: wrap the buffer in a
std::span, take asubspanper row/lane, and use range-based algorithms instead of manual pointer arithmetic.math/operations.cpp—sigmoid/thresholdbecomestd::ranges::transform;softmax/argmaxtake a per-lanesubspaninstead of re-deriving offsets from a raw pointer on each access.cv/image_ops.cpp— channel-plane and colormap-pixel access go through subspans; thetoChannelsFirstmemcpybecomesstd::ranges::copy.cv/box_ops.cpp—decodeToXyxytakes astd::span<const float, 4>row rather than four loose floats, removing theboxesPtr[idx * 4 + 0..3]unpacking at both call sites.Byte-level spans are bounded by
size_(bytes) and element spans bynumel_, so everysubspanis bounds-checked against the real allocation.cv::Matconstructions that takedata_.get()are left as-is: they hand the whole buffer to OpenCV rather than doing pointer math, so a span adds nothing. Core files are untouched, per the issue.Also drops the five repeated
if (!result.ok())blocks innlp/tokenizer.cppin favour of a localunwraphelper, mirroring the one incore/model.cpp(that one is file-local and typed toexecutorch::runtime::Result, so it cannot be reused fortokenizers::Result).Introduces a breaking change?
Type of change
Tested on
Testing instructions
Refactor is behaviour-preserving, and verification so far is compile-only — not yet run on a device.
All four translation units compile clean under the project's strict warning set from
.clangd(-Wall -Wextra -Wpedantic -Wconversion -Wsign-conversion -Wold-style-cast -Wcast-align -Wshadow -Wdouble-promotion), with no new warnings:To exercise at runtime: run
apps/computer-vision(classification/segmentation/detection coversoftmax,argmax,applyColormap,resize,normalize,toChannelsFirst, andnms) and the tokenizer screen inapps/nlp.Related issues
Closes #1308
Checklist
Additional notes