Fill LSTM CUDA operator opset gap: extend coverage from opset 14 to opset 22 - #27737
Conversation
Cap existing opset 14 non-versioned LSTM kernel to versioned (14-21), add new non-versioned LSTM kernel at opset 22, and update forward declarations and BuildKernelCreateInfo entries in cuda_execution_provider.cc. Add opset 22 LSTM CUDA test. Co-authored-by: tianleiwu <30328909+tianleiwu@users.noreply.github.com>
|
/azp run Windows GPU Doc Gen CI Pipeline |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Copilot, please update docs/OperatorKernels.md, and merge latest main branch to this branch. |
…d merge latest main Co-authored-by: tianleiwu <30328909+tianleiwu@users.noreply.github.com>
Tianlei Wu (tianleiwu)
left a comment
There was a problem hiding this comment.
Review Summary
Clean, low-risk opset gap-fill that extends CUDA LSTM kernel registration from opset 14 to cover opsets 14–21 (versioned) and 22+ (non-versioned). The pattern exactly matches the existing GRU and RNN registrations. Since the ONNX LSTM spec has no functional changes between opset 14 and 22 (the opset-22 doc string is literally kDoc_LSTM_ver14), the same kernel implementation handles all versions correctly.
Positives:
- Registration macros in
lstm.ccfollow the established three-macro structure fromgru.ccandrnn.cc(REGISTER_KERNEL_VERSIONED_TYPEDfor 7–13,REGISTER_KERNEL_VERSIONED_TYPED_14for 14–21,REGISTER_KERNEL_TYPEDfor 22+). - Forward declarations and
BuildKernelCreateInfoentries incuda_execution_provider.ccare placed adjacent to GRU/RNN entries, maintaining the ordering convention. docs/OperatorKernels.mdcorrectly updated.- Test properly guards with
DefaultCudaExecutionProvider()check and validates behavior equivalence against opset 14 expected values.
Dmitri Smirnov (yuslepukhin)
left a comment
There was a problem hiding this comment.
In cudnn_rnn_base.cc, multiple size computations use raw int64_t multiplication with no overflow guard:
// Line ~109: weight buffer size
int64_t w_size = num_directions_ * (number * hidden_size_ * (input_size + hidden_size_ + 2));
// Line ~254: output buffer size
int64_t output_size = seq_length * num_directions_ * batch_size * hidden_size_;
// Line ~240: reverse buffer
GetScratchBuffer(seq_length * batch_size * input_size, ...)
There was a problem hiding this comment.
The code uses gsl::narrow_cast<int32_t>(seq_length) etc. throughout ComputeInternal. narrow_cast is an unchecked cast — it does NOT throw on truncation (unlike gsl::narrow). If seq_length, batch_size, or hidden_size exceed INT32_MAX, this silently truncates. Given these values come from user-controlled tensor shapes, gsl::narrow would be safer.
There was a problem hiding this comment.
No explicit rank validation on input X.
f X has rank < 3, this is an out-of-bounds read on the shape vector. The ONNX schema should enforce rank-3, but an explicit ORT_RETURN_IF(X->Shape().NumDimensions() != 3, ...) guard would be defensive
int64_t seq_length = X->Shape()[0];
int64_t batch_size = X->Shape()[1];
int64_t input_size = X->Shape()[2];
There was a problem hiding this comment.
SetZeroSequences takes zero_seq_index_cache by value.
This copies the entire vector on every call. Should be const std::vector<int32_t>&. This is a performance issue, not a correctness bug.
Coding standards require passing gsl::span in such cases.
Dmitri Smirnov (yuslepukhin)
left a comment
There was a problem hiding this comment.
Critical: The CUDA EP silently produces wrong results when input_forget=1 or when peephole weights P are provided. It should either implement the feature or return INVALID_ARGUMENT / decline the node so it falls back to CPU. This is the most serious functional correctness gap — the kernel claims the node but computes the wrong result.
Addressing Review FeedbackAll four review-level concerns from Dmitri Smirnov (@yuslepukhin) on 1. Overflow in size computationsAdded
2.
|
- Add SafeInt overflow guards for size computations (w_size, output_size, scratch buffer sizes) - Replace gsl::narrow_cast with checked gsl::narrow for int32_t casts - Add rank-3 validation on input tensor X - Change SetZeroSequences to take gsl::span<const int32_t> instead of vector by value - Fix test W_data shape mismatch (input_size 2→1 to match 8-element W) - Add Y (full sequence) output validation alongside Y_h
Dmitri Smirnov (yuslepukhin)
left a comment
There was a problem hiding this comment.
Both CPU and CUDA: OOB read before rank check on X
In lstm_base.cc:43-48:
int seq_length = narrow(X_shape[0]); // OOB if rank < 3
int batch_size = narrow(X_shape[1]);
int input_size = narrow(X_shape[2]);
Status status = ValidateInputs(X, ...); // rank check is HERE, too late
Same pattern in cudnn_rnn_base.cc:190-192:
int64_t seq_length = X->Shape()[0]; // No rank check anywhere
int64_t batch_size = X->Shape()[1];
int64_t input_size = X->Shape()[2];
Dmitri Smirnov (yuslepukhin)
left a comment
There was a problem hiding this comment.
Neither implementation validates W or R shapes:
W and R are not validated. No check that:
W.rank == 3, W[0] == num_directions, W[1] == 4hidden_size, W[2] == input_size
R.rank == 3, R[0] == num_directions, R[1] == 4hidden_size, R[2] == hidden_size.
In deep_cpu_lstm.cc:300-301, W_shape[1], W_shape[2] are accessed without rank validation — OOB if W is rank < 3.
Multiplication is not SafeInt-guarded (it's int64_t * size_t — standard arithmetic). Should be SafeInt<size_t>(w_size) * sizeof(T). Refers to: onnxruntime/core/providers/cuda/rnn/cudnn_rnn_base.cc:112 in a08329d. [](commit_id = a08329d, deletion_comment = False) |
Same issue — output_size is SafeInt-computed but the final * sizeof(T) is raw multiplication. Refers to: onnxruntime/core/providers/cuda/rnn/cudnn_rnn_base.cc:396 in a08329d. [](commit_id = a08329d, deletion_comment = False) |
CacheCudnnRnnWeights accesses W->Shape()[2] without rank check — cudnn_rnn_base.cc:153. The W/R rank check was added in ReorganizeWeights, but CacheCudnnRnnWeights also accesses W->Shape()[2] directly (line 153: tmp_rnn_desc.Set(W->Shape()[2], ...)) without going through ReorganizeWeights first. If W is a constant input with rank < 3, this is an OOB read in the constructor path. Refers to: onnxruntime/core/providers/cuda/rnn/cudnn_rnn_base.cc:148 in a08329d. [](commit_id = a08329d, deletion_comment = False) |
|
Addressing all review feedback from Dmitri Smirnov (@yuslepukhin) — here's the status of each item on the current HEAD (
|
Dmitri Smirnov (yuslepukhin)
left a comment
There was a problem hiding this comment.
LGTM
Description
Extends LSTM CUDA kernel registration from opset 14 to opset 22.
lstm.cc: Cap existing opset 14 kernel to versioned 14–21, add new non-versioned kernel at opset 22cuda_execution_provider.cc: Update forward declarations andBuildKernelCreateInfoentries accordingly (versioned 14–21 + non-versioned 22) for all three types (float,double,MLFloat16)deep_cpu_lstm_op_test.cc: AddONNXRuntime_TestLSTMForward_OpSet22_CUDAtest targeting the new registrationdocs/OperatorKernels.md: Update CUDA LSTM entry from14+to[14, 21]and22+No spec-level behavior changes between opsets 14 and 22 for LSTM — this is purely a registration gap fill so the CUDA EP correctly claims nodes exported at newer opset versions.
Motivation and Context
LSTM CUDA kernel was registered only up to opset 14 while the ONNX spec defines LSTM through opset 22. Models exported at opset ≥15 would fall back to CPU. Follows the same pattern established by other opset gap PRs (ConvTranspose, MaxPool, Pad, etc.) referenced in #27729.
📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.