-
Notifications
You must be signed in to change notification settings - Fork 205
fix: make fill_next_token_bitmask stride-aware #699
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -189,7 +189,29 @@ int32_t* CheckAndGetBitmaskPtr(const DLTensor& token_bitmask, int vocab_size, in | |
| token_bitmask.device.device_type == kDLROCMHost | ||
| ) << "The provided bitmask's device is not valid: should be CPU"; | ||
|
|
||
| return reinterpret_cast<int32_t*>(token_bitmask.data) + index * buffer_size; | ||
| // The bitmask may be a non-contiguous view, so the row offset must follow strides[0] instead | ||
| // of assuming a compact layout: ApplyMask32Bits addresses rows the same way, and filling and | ||
| // applying must agree on where a row lives. Null strides means compact (DLPack). The vocab | ||
| // dimension must stay unit-stride, since a row is read as one contiguous DynamicBitset. | ||
| int64_t row_stride = buffer_size; | ||
| if (token_bitmask.strides != nullptr) { | ||
| int64_t vocab_stride = token_bitmask.strides[token_bitmask.ndim - 1]; | ||
| XGRAMMAR_CHECK(vocab_stride == 1) | ||
| << "The provided bitmask must be contiguous along the vocabulary dimension, but got " | ||
| "stride " | ||
| << vocab_stride; | ||
| if (token_bitmask.ndim == 2) { | ||
| row_stride = token_bitmask.strides[0]; | ||
| // A row spans buffer_size int32s; with more than one row a smaller stride would overlap | ||
| // adjacent rows and let a write run past the buffer. A single row cannot overlap, whatever | ||
| // its stride, so only guard when rows > 1. Reject rather than corrupt memory. | ||
| XGRAMMAR_CHECK(token_bitmask.shape[0] <= 1 || row_stride >= buffer_size) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto |
||
| << "The row stride of the bitmask must be at least " << buffer_size | ||
| << " so rows do not overlap, but got " << row_stride; | ||
| } | ||
| } | ||
|
Seven-Streams marked this conversation as resolved.
|
||
|
|
||
| return reinterpret_cast<int32_t*>(token_bitmask.data) + index * row_stride; | ||
| } | ||
|
|
||
| void _DebugGetMaskedTokensFromBitmask( | ||
|
|
@@ -225,8 +247,25 @@ void ApplyMask32Bits( | |
| logits->ndim == 2 | ||
| ? std::make_pair(static_cast<int>(logits->shape[0]), static_cast<int>(logits->shape[1])) | ||
| : std::make_pair(1, static_cast<int>(logits->shape[0])); | ||
| int logits_stride0 = logits->strides[0]; | ||
| int bitmask_stride0 = bitmask.strides[0]; | ||
| // Null strides means compact (DLPack), in which case a row is one shape[-1]-long span. | ||
| int logits_stride0 = | ||
| logits->strides != nullptr ? static_cast<int>(logits->strides[0]) : logits_shape.second; | ||
| int bitmask_stride0 = bitmask.strides != nullptr | ||
| ? static_cast<int>(bitmask.strides[0]) | ||
| : static_cast<int>(bitmask.shape[bitmask.ndim - 1]); | ||
| // Mirror the fill-side check: with more than one row, a row stride smaller than the per-row | ||
| // span (shape[-1]) overlaps adjacent rows -- corrupting on the logits -inf write. A single row | ||
| // cannot overlap and legitimately carries an unconstrained stride, so only guard when rows > 1. | ||
| if (logits_shape.first > 1) { | ||
| XGRAMMAR_CHECK(logits_stride0 >= logits_shape.second) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto. xgrammar_check is used to check the correctness of inner logic, while
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks — and no objection to the change itself: Before I push it, though, I want to check one thing, because the header documents the mapping the other way round. In
Those comments came in with #354, and internal assertions in So I'd rather not guess. Which do you mean?
Happy with any of them — just say which and I'll push accordingly. |
||
| << "The row stride of the logits must be at least " << logits_shape.second | ||
| << " so rows do not overlap, but got " << logits_stride0; | ||
| } | ||
| if (bitmask.ndim == 2 && bitmask.shape[0] > 1) { | ||
| XGRAMMAR_CHECK(bitmask_stride0 >= static_cast<int>(bitmask.shape[1])) | ||
| << "The row stride of the bitmask must be at least " << bitmask.shape[1] | ||
| << " so rows do not overlap, but got " << bitmask_stride0; | ||
| } | ||
| if (indices.has_value()) { | ||
| for (auto idx : indices.value()) { | ||
| uint32_t* data_ptr = reinterpret_cast<uint32_t*>(bitmask.data) + idx * bitmask_stride0; | ||
|
Seven-Streams marked this conversation as resolved.
|
||
|
|
@@ -274,8 +313,25 @@ void ApplyMask16Bits( | |
| logits->ndim == 2 | ||
| ? std::make_pair(static_cast<int>(logits->shape[0]), static_cast<int>(logits->shape[1])) | ||
| : std::make_pair(1, static_cast<int>(logits->shape[0])); | ||
| int logits_stride0 = logits->strides[0]; | ||
| int bitmask_stride0 = bitmask.strides[0]; | ||
| // Null strides means compact (DLPack), in which case a row is one shape[-1]-long span. | ||
| int logits_stride0 = | ||
| logits->strides != nullptr ? static_cast<int>(logits->strides[0]) : logits_shape.second; | ||
| int bitmask_stride0 = bitmask.strides != nullptr | ||
| ? static_cast<int>(bitmask.strides[0]) | ||
| : static_cast<int>(bitmask.shape[bitmask.ndim - 1]); | ||
| // Mirror the fill-side check: with more than one row, a row stride smaller than the per-row | ||
| // span (shape[-1]) overlaps adjacent rows -- corrupting on the logits -inf write. A single row | ||
| // cannot overlap and legitimately carries an unconstrained stride, so only guard when rows > 1. | ||
| if (logits_shape.first > 1) { | ||
| XGRAMMAR_CHECK(logits_stride0 >= logits_shape.second) | ||
| << "The row stride of the logits must be at least " << logits_shape.second | ||
| << " so rows do not overlap, but got " << logits_stride0; | ||
| } | ||
| if (bitmask.ndim == 2 && bitmask.shape[0] > 1) { | ||
| XGRAMMAR_CHECK(bitmask_stride0 >= static_cast<int>(bitmask.shape[1])) | ||
| << "The row stride of the bitmask must be at least " << bitmask.shape[1] | ||
| << " so rows do not overlap, but got " << bitmask_stride0; | ||
| } | ||
| if (indices.has_value()) { | ||
| for (auto idx : indices.value()) { | ||
| uint32_t* data_ptr = reinterpret_cast<uint32_t*>(bitmask.data) + idx * bitmask_stride0; | ||
|
Seven-Streams marked this conversation as resolved.
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can use
xgrammar_log(fatal)here? Since it's a user's input error.