[WebGPU EP] Add AveragePool 19/22 and integer Clip kernel registrations - #29835
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
@adrastogi Please help trigger the copilot review and testing workflows, thanks! |
There was a problem hiding this comment.
Pull request overview
This PR improves WebGPU Execution Provider (EP) kernel coverage and correctness to prevent unintended CPU fallbacks (and associated GPU↔CPU copies / compile-only aborts). It expands WebGPU kernel registrations for AveragePool across missing opset ranges, adds WebGPU registrations for integer Clip (int32/uint32), and updates the WebGPU AveragePool shader to compute the count_include_pad divisor correctly under ceil_mode.
Changes:
- Register
AveragePoolWebGPU kernels for opsets 11–18, 19–21, and 22 (both ONNX and internal NHWC domains). - Register WebGPU
Clipkernels forint32_t/uint32_tstarting at opset 12 (and opset 13+ via the unversioned registration). - Add WebGPU-targeted unit tests covering the new registration ranges and the
AveragePoolceil_mode + count_include_padbehavior.
Show a summary per file
| File | Description |
|---|---|
| onnxruntime/core/providers/webgpu/webgpu_execution_provider.cc | Adds kernel create-info entries for integer Clip and version-split AveragePool registrations. |
| onnxruntime/core/providers/webgpu/nn/pool.cc | Splits AveragePool registrations by opset range and adjusts shader generation for count_include_pad under ceil_mode. |
| onnxruntime/core/providers/webgpu/math/unary_elementwise_ops.cc | Adds int32_t/uint32_t WebGPU Clip registrations starting at opset 12. |
| onnxruntime/test/providers/cpu/nn/pool_op_test.cc | Adds WebGPU EP tests validating AveragePool opset registrations (11, 19, 22) and the divisor fix scenario. |
| onnxruntime/test/providers/cpu/math/clip_test.cc | Adds WebGPU EP tests for integer Clip at opsets 12 and 13. |
Review details
Comments suppressed due to low confidence (1)
onnxruntime/core/providers/webgpu/nn/pool.cc:118
- The small-output/big-kernel (workgroup reduction) path still divides by
count_shared[0]without checking for zero. The same dilations edge case (no in-bounds taps) can yieldcount_shared[0]==0and NaN output. Add a guard so that when the reduced count is 0, the output stays 0 (matching CPU reference behavior).
counting_code = count_include_pad_
? " if (!is_outside_padded_region) {\n count++;\n }\n"
: " if (!is_pad) {\n count++;\n }\n";
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Low
Review: PR #29835 — [WebGPU EP] Add AveragePool 19/22 and integer Clip kernel registrations (head
|
signed_pos range |
Region | Inside padded region? |
|---|---|---|
signed_pos < 0 |
begin padding | Yes |
0 <= signed_pos < j_dim_len |
real input | Yes |
j_dim_len <= signed_pos < j_dim_len + pad_end |
end padding | Yes |
signed_pos >= j_dim_len + pad_end |
ceil_mode overflow | No |
The check if (signed_pos >= i32(j_dim_len) + i32(pad_end)) flags only the last category. Correct.
The comment "only the end side can overflow, since the smallest window position is -pad_begin" is right: y_indices[j] is a non-negative u32, k_indices is non-negative, and k_indices * dilation - pad_begin at the smallest k_indices=0 gives -pad_begin — which is ≥ the begin-padding start. So the begin side is never past the padded region, only the end can be. The signed arithmetic guards against u32 underflow for the begin side (u32 window indices underflow for begin-pad cells that are inside the padded region, but signed i32 handles this cleanly).
counting_code truth table:
| mode | condition | count contribution |
|---|---|---|
count_include_pad=true |
!is_outside_padded_region |
1 for real + real-padding, 0 for ceil-overflow |
count_include_pad=false |
!is_pad |
1 only for real input |
Matches ONNX AveragePool semantics for both count_include_pad values. ✓
The two Copilot findings — both legitimate
🔴 HIGH: Div-by-zero on dilated pools when the window has zero in-bounds taps
count starts at 0 and is only incremented inside the sampling loop. Two failure modes:
count_include_pad=false,count=0: if a window is entirely outside the real input (all taps hit padding),count++never fires. Can happen with dilation > 1 at the input edge, or degenerate configs. CPU reference leaves the output at 0; this shader will divide by zero and produce NaN.count_include_pad=true,count=0: only fires when the window is entirely in the ceil-overflow region (beyond the padded input). More exotic, but same failure mode.
Fix (small, localized):
let output = select(f32(0), value / f32(count), count > 0u);or equivalent select at the divisor site. Copilot notes this issue "also appears on line 116" — likely the f16 variant of the same divide. Both need the same guard.
The bug wasn't exposed before this PR because (a) count_include_pad=false used the compile-time uniforms.kernel_size divisor at line ~103 pre-refactor and (b) count_include_pad=true also used the constant. Neither ever hit zero. Now that both modes accumulate count, both can produce 0. Worth guarding before merge.
🟡 MEDIUM: Lost early break on non-overflow paths
The old bounds-check block:
if (x_indices[j] < 0 || x_indices[j] >= j_dim_len) {
is_pad = true;
break; // <-- removed
}The break is removed unconditionally so that overflow_check_code can run for every spatial dimension. But overflow_check_code is empty in three of four combinations:
- MaxPool (no overflow check).
- AveragePool +
count_include_pad=false(no overflow check). - AveragePool +
count_include_pad=true(the only path that needs it).
For the first two, the loop now iterates through all remaining spatial dimensions on every padded tap, doing nothing useful. For a 5×5 or 7×7 MaxPool with padded output edges, that's 2–4× more loop iterations per boundary tap.
Fix (also small):
const std::string bounds_check_body = overflow_check_code.empty()
? " is_pad = true;\n break;\n"
: " is_pad = true;\n";and interpolate bounds_check_body where the current inline text lives. Alternatively, gate the early break on a boolean parameter passed to the shader-code emitter. Either preserves the pre-PR fast path for the three non-affected combinations.
Registration completeness (verified)
- AveragePool: 3 new NCHW + 3 new NHWC = 6 entries in webgpu_execution_provider.cc, matching 6 macro calls in pool.cc. No class-declaration orphans.
- Integer Clip: 4 new entries (int32 12-12, int32 13+, uint32 12-12, uint32 13+), matching
WEBGPU_CLIP_KERNEL_FROM_12(int32_t)+WEBGPU_CLIP_KERNEL_FROM_12(uint32_t)in unary_elementwise_ops.cc. No orphans. - The 84/86 CI result implies the build is fine (2 pending or Azure-Pipelines-not-run legs).
Test coverage
5 new tests, all with WebGPU EP availability skip guards:
Clip_int32_WebGpuat opset 12 — exercises the 12-12 registration.Clip_uint32_WebGpuat opset 13 — exercises the 13+ unversioned registration.AveragePool_19_ceil_count_include_pad_1d_WebGpu— the important one. Kernel=7, stride=3, pad={3,3}, ceil_mode=1, count_include_pad=1, input length 9. Output length is 4 (ceil((9+6-7)/3)+1). The last window (y=3) coverssigned_pos = 6..12, of whichsigned_pos=12is the ceil-overflow cell. Correct divisor for that window is 6, not 7. Test pins the fix.AveragePool_11_1d_WebGpu— exercises the 11-18 registration.AveragePool_22_1d_WebGpu— exercises the 22+ registration.
Good coverage. Consistent skip pattern (all five tests skip when WebGPU EP isn't in the build — better than PR #29833 which was inconsistent).
Missing from the coverage: no test exercises the div-by-zero path Copilot flagged. If author adds the count > 0 guard, a small test using dilation and a window that lands entirely outside the input would lock it in. Not strictly required (that config is rare), but it would prevent the div-by-zero from silently regressing later.
Minor observations
- The added blank line before
WEBGPU_CLIP_KERNELin unary_elementwise_ops.cc is cosmetic — fine. - The comment on
WEBGPU_CLIP_KERNEL_FROM_12explicitly justifies the opset-11 omission with the schema constraint. Good defensive documentation for a maintainer who might otherwise "fix" the missing opset-11 registration and hit a schema error. - The
overflow_check_codeis stitched into the shader as a string. When it's empty (three of four combos), stitching an empty string is a no-op — no compile issue, just wasted stream ops. Not worth optimizing. outside_flag_decland the reference tois_outside_padded_regionincounting_codeare gated by the same!is_max_pool_ && count_include_pad_condition — so the flag is always declared when it's referenced. No dangling identifier.- The two-line comment above the bounds check (
"(No early break: the count_include_pad overflow check below must run for every spatial dimension.)") is accurate for the AveragePool+count_include_pad case but misleading for the other three cases where no overflow check runs. If you take the Medium fix, this comment should be reworked to say "conditional early break" or similar.
Bottom line
The three coverage/correctness changes are good and land the right fixes for the right reasons. The two Copilot findings on the pool-shader divisor implementation are legitimate and both have small, localized fixes:
- Must-fix (HIGH):
count == 0division guard in the AveragePool divide (both f32 and f16 paths). - Should-fix (MEDIUM): conditional early break so MaxPool and AveragePool+
!count_include_paddon't pay for the overflow-check machinery they never invoke.
With those addressed, this is a clean approve. The registration side (AveragePool opsets 19/22, integer Clip opsets 12/13+) has no issues at all — that half of the PR can land as-is.
417b2d9 to
7973da9
Compare
|
@hariharans29 |
Register missing opset ranges for existing WebGPU kernels so these nodes stop falling back to the CPU EP (extra GPU<->CPU copies; session-init abort under the compile-only flow). * AveragePool (pool.cc, webgpu_execution_provider.cc): register opsets 11-18, 19-21 and 22, replacing the single opset-11 registration. ORT clamps an open-ended SinceVersion(11) registration to [11, 18] (not [11, INT_MAX]), so a model at opset 21 (AveragePool SinceVersion 19) was not claimed and fell back to CPU; the CPU-only NchwcTransformer (Level3) then amplified the single pool into 3 CPU nodes. * Clip int32/uint32 (unary_elementwise_ops.cc, webgpu_execution_provider.cc): register the 4-byte templated Clip for int32/uint32 at opsets 12-12 and 13, via a dedicated WEBGPU_CLIP_KERNEL_FROM_12 macro. Integer types are only valid Clip element types from opset 12 on (the opset-11 Clip schema constrains T to float types), so there is no 11-11 integer registration. The clip min/max are carried in the f32-typed `attr` uniform: for integer types the values are bit-reinterpreted to f32 so the supplied uniform value's data type matches the declaration (full-validation mode rejects a mismatch), and the shader recovers them with bitcast. Used by shape/index/mask subgraphs (e.g. an index/position path). * AveragePool count_include_pad divisor (pool.cc): a pre-existing WebGPU pool-kernel bug exposed by the opset-19 registration. With count_include_pad the shader divided by the full kernel size, which is wrong under ceil_mode (the last window can extend past the padded region; those overflow cells must not be counted). Now count is accumulated in the kernel loop and, under count_include_pad, excludes cells beyond the padded region (signed arithmetic, since u32 window indices underflow for begin padding, which is inside the region). The divide is guarded against an all-padding window (count == 0) so the output stays 0 instead of dividing by zero, and the per-cell bounds check breaks out of the dimension loop early except when the count_include_pad overflow check must visit every spatial dimension. Tests (explicit WebGPU EP): clip_test.cc Clip_int32_WebGpu (opset 12) / Clip_uint32_WebGpu (opset 13); pool_op_test.cc AveragePool_11_1d_WebGpu, AveragePool_19_ceil_count_include_pad_1d_WebGpu (pins the divisor fix) and AveragePool_22_1d_WebGpu, one per registration range.
7973da9 to
e4e91d7
Compare
Re-review: PR #29835 — [WebGPU EP] Add AveragePool 19/22 and integer Clip kernel registrations (head
|
| Combination | overflow_check_code |
pad_break_code |
Result |
|---|---|---|---|
| MaxPool | "" |
"break;\n" | Early break preserved ✓ |
AveragePool + !count_include_pad |
"" |
"break;\n" | Early break preserved ✓ |
AveragePool + count_include_pad |
signed-arith overflow check | "" |
Every dim visited (needed for divisor correctness) ✓ |
| — (there is no fourth case here) |
Preserves the pre-PR fast path for the two combinations that never needed the overflow-check machinery, and correctly disables the early break only for the one combination that does need per-dimension iteration.
The stale comment "(No early break: the count_include_pad overflow check below must run for every spatial dimension.)" has been reworked to be conditional-accurate:
// ------ Check if x_indices[j] is out of bounds to handle padding. Once a cell is known to be
// padding, later dimensions can't change that, so break early -- EXCEPT under
// AveragePool + count_include_pad, where the overflow check below must run for every
// spatial dimension (pad_break_code is empty in that case).Clear and accurate for all four combinations.
Bonus cleanup: integer-Clip attr encoding
The if constexpr cascade in onnxruntime/core/providers/webgpu/math/unary_elementwise_ops.cc's Clip template was split from MLFloat16 / else into MLFloat16 / float / else (i32/u32), with the integer branch getting its own comment explaining the ORT-side "declared f32, populated with reinterpreted integer bits, recovered via bitcast<x_element_t> in the shader" contract:
} else if constexpr (std::is_same_v<T, float>) {
// f32: stored as-is.
program.AddUniformVariable({gsl::make_span(attr, 2)});
} else {
// i32 / u32: the "attr" uniform is declared f32 and the WebGPU EP validates that the supplied
// uniform value's data type matches the declaration. Reinterpret the integer bits as f32 so the
// types match; the shader recovers the integer values with bitcast<x_element_t>(uniforms.attr[i]).
static_assert(sizeof(T) == sizeof(float), "integer Clip attr must be 4 bytes");
float encoded[2];
std::memcpy(encoded, attr, sizeof(encoded));
program.AddUniformVariable({gsl::make_span(encoded, 2)});
}Correctness of the reinterp:
- memcpy(encoded, attr, 8) copies
sizeof(T[2]) = 8 bytesintofloat[2] = 8 bytes. ✓ - WGSL
bitcast<i32>(f32)/bitcast<u32>(f32)is defined as a pure bit reinterpretation across 32-bit scalar types (no value-domain conversion). ✓ - Signaling-NaN concern: uniforms of
f32declared type carry raw 32-bit bit patterns; the WebGPU runtime doesn't do value validation on the bits (only ORT's AddUniformVariable(float[]) type check, which is by declared type, not by value class). So an integer bit pattern that would be an sNaN as float goes through untouched. ✓
Good defensive comment for a future maintainer who might not immediately see why the code is packing int bits into float uniform slots.
Registration side (unchanged from prior pass, still verified)
Cross-checked against onnxruntime/core/providers/webgpu/webgpu_execution_provider.cc: 6 new AveragePool BuildKernelCreateInfo entries (3 NCHW × opsets [11, 18] / [19, 21] / 22+, plus 3 NHWC mirrors), and 4 new integer-Clip entries (int32_t at opsets [12, 12] / 13+, plus uint32_t mirrors). Each provider-table entry has a matching macro invocation in onnxruntime/core/providers/webgpu/nn/pool.cc / onnxruntime/core/providers/webgpu/math/unary_elementwise_ops.cc. No class-declaration orphans.
Version-range choices match ONNX schema history: AveragePool got new schema versions at opsets 11, 19, 22; integer Clip is only valid from opset 12 onward.
Test coverage (unchanged, 5 tests)
Clip_int32_WebGpu(opset 12) → exercises 12-12 registration.Clip_uint32_WebGpu(opset 13) → exercises 13+ unversioned registration.AveragePool_19_ceil_count_include_pad_1d_WebGpu(opset 19) → exercises 19-21 registration and locks in thecount_include_pad+ceil_modedivisor fix. The last window (y=3,signed_pos = 6..12withj_dim_len + pad_end = 12) exercises exactly thesigned_pos == 12ceil-overflow cell that must be excluded from the divisor.AveragePool_11_1d_WebGpu(opset 11) → exercises 11-18 registration.AveragePool_22_1d_WebGpu(opset 22) → exercises 22+ registration.
All five use the same DefaultWebGpuExecutionProvider()-availability skip pattern (consistent with the pattern established across the family of WebGPU-registration PRs).
Still-non-blocking follow-up
No new test explicitly exercises the div-by-zero guard (dilation + entirely-out-of-input window). Not required, since the guard is a defensive shim over an already-rare shape, but a locked-in test would prevent regression if someone later "simplifies" the divide back to unguarded. Fine to leave for a follow-up.
CI
- Head
e4e91d7: 85 / 86 OK. One leg pending or unrelated. Nothing in the change surface (pool-shader emitter + kernel registrations + tests) is a plausible break source for the wider CI matrix.
Bottom line
The two Copilot findings from the last pass are both properly addressed at the correct sites — the div-by-zero guard is applied in both divide-emission code paths and the conditional early break preserves the pre-PR fast path for MaxPool and AveragePool-without-count_include_pad. The integer-Clip attr-encoding cleanup is a small readability win. Registration side, test set, and shader semantics for the divisor fix are all unchanged from the prior good state. Ready to merge after the one still-pending CI leg reports green.
Register missing opset ranges for existing WebGPU kernels so these nodes stop falling back to the CPU EP (extra GPU<->CPU copies; session-init abort under the compile-only flow).
AveragePool (pool.cc, webgpu_execution_provider.cc): register opsets 11-18, 19-21 and 22, replacing the single opset-11 registration. ORT clamps an open-ended SinceVersion(11) registration to [11, 18] (not [11, INT_MAX]), so MobileNet (opset 21, AveragePool SinceVersion 19) was not claimed and fell back to CPU; the CPU-only NchwcTransformer (Level3) then amplified the single pool into 3 CPU nodes.
Clip int32/uint32 (unary_elementwise_ops.cc, webgpu_execution_provider.cc): register the 4-byte templated Clip for int32/uint32 at opsets 12-12 and 13, via a dedicated WEBGPU_CLIP_KERNEL_FROM_12 macro. Integer types are only valid Clip element types from opset 12 on (the opset-11 Clip schema constrains T to float types), so there is no 11-11 integer registration. Used by shape/index/mask subgraphs (e.g. CLIP text position ids).
AveragePool count_include_pad divisor (pool.cc): a pre-existing WebGPU pool-kernel bug exposed by the opset-19 registration. With count_include_pad the shader divided by the full kernel size, which is wrong under ceil_mode (the last window can extend past the padded region; those overflow cells must not be counted). Now count is accumulated in the kernel loop and, under count_include_pad, excludes cells beyond the padded region (signed arithmetic, since u32 window indices underflow for begin padding, which is inside the region).
This is a split CL from #29682 to fix part of #29756