[WebGPU] Add Max and Min operators - #29833
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
@qjia7 @hariharans29 PTAL |
There was a problem hiding this comment.
Pull request overview
This PR adds native WebGPU Execution Provider support for ONNX Max and Min, including variadic-input handling and NaN-propagation behavior required by opset 12+ so graphs avoid falling back to the CPU EP.
Changes:
- Refactors the existing binary elementwise implementation by extracting a reusable
RunBinaryProgram()helper. - Adds a
VariadicElementwiseWebGPU kernel that folds 1..N inputs with NumPy-style broadcasting (used forMax/Min). - Registers WebGPU
Max/Minkernels across opset ranges and adds WebGPU-focused NaN propagation tests.
Show a summary per file
| File | Description |
|---|---|
| onnxruntime/test/providers/cpu/math/element_wise_ops_test.cc | Adds WebGPU-specific NaN propagation tests for Max/Min (opset 12). |
| onnxruntime/core/providers/webgpu/webgpu_execution_provider.cc | Registers WebGPU kernel create-info entries for Max/Min across opset ranges. |
| onnxruntime/core/providers/webgpu/math/binary_elementwise_ops.{h,cc} | Introduces VariadicElementwise, factors out RunBinaryProgram(), and implements WGSL NaN-propagating max/min helpers. |
Review details
Comments suppressed due to low confidence (4)
onnxruntime/test/providers/cpu/math/element_wise_ops_test.cc:2484
- DefaultWebGpuExecutionProvider() returns nullptr when ORT is built without USE_WEBGPU, so this test will try to run with a null EP and fail in non-WebGPU builds. Add a runtime skip (or compile-time guard) when the WebGPU EP isn't available.
std::vector<std::unique_ptr<IExecutionProvider>> execution_providers;
execution_providers.push_back(DefaultWebGpuExecutionProvider());
test.Run(OpTester::ExpectResult::kExpectSuccess, "", {}, nullptr, &execution_providers);
onnxruntime/test/providers/cpu/math/element_wise_ops_test.cc:2500
- DefaultWebGpuExecutionProvider() returns nullptr when ORT is built without USE_WEBGPU, so this test will try to run with a null EP and fail in non-WebGPU builds. Add a runtime skip (or compile-time guard) when the WebGPU EP isn't available.
std::vector<std::unique_ptr<IExecutionProvider>> execution_providers;
execution_providers.push_back(DefaultWebGpuExecutionProvider());
test.Run(OpTester::ExpectResult::kExpectSuccess, "", {}, nullptr, &execution_providers);
onnxruntime/test/providers/cpu/math/element_wise_ops_test.cc:2514
- DefaultWebGpuExecutionProvider() returns nullptr when ORT is built without USE_WEBGPU, so this test will try to run with a null EP and fail in non-WebGPU builds. Add a runtime skip (or compile-time guard) when the WebGPU EP isn't available.
std::vector<std::unique_ptr<IExecutionProvider>> execution_providers;
execution_providers.push_back(DefaultWebGpuExecutionProvider());
test.Run(OpTester::ExpectResult::kExpectSuccess, "", {}, nullptr, &execution_providers);
onnxruntime/test/providers/cpu/math/element_wise_ops_test.cc:2531
- DefaultWebGpuExecutionProvider() returns nullptr when ORT is built without USE_WEBGPU, so this test will try to run with a null EP and fail in non-WebGPU builds. Add a runtime skip (or compile-time guard) when the WebGPU EP isn't available.
std::vector<std::unique_ptr<IExecutionProvider>> execution_providers;
execution_providers.push_back(DefaultWebGpuExecutionProvider());
test.Run(OpTester::ExpectResult::kExpectSuccess, "", {}, nullptr, &execution_providers);
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Low
Review: PR #29833 — [WebGPU] Add Max and Min operators (head
|
| a_nan | b_nan | result |
|---|---|---|
| F | F | builtin(a,b) |
| F | T | b (NaN) |
| T | F | a (NaN) |
| T | T | a (NaN) |
Every NaN operand produces a NaN output. ✓ (Both-NaN returns a, but any NaN is fine — ONNX doesn't distinguish which NaN.)
f16 widening: A vec4<f16> is 8 bytes; vec4<u32> is 16 bytes. Direct bitcast<vec4<u32>>(vec4<f16>) doesn't compile. The code widens: bitcast<vec4<u32>>(vec4<f32>(a)). f16 → f32 is lossless (all f16 values are exactly representable in f32, and NaN is preserved through the widen). The > 0x7f800000u check then correctly detects f32-NaN as it would for any f32. Correct.
Integer path: if (is_float) gates the whole bitcast machinery; integer types get the plain min/max builtin. Correct — integers can't be NaN.
Test coverage
5 new WebGPU tests in element_wise_ops_test.cc:
Max_12_Float_Nan_WebGpu— 3×3 with 3 NaNs broadcast against 3×1 (vec4 broadcast shader path).Min_12_Float_Nan_WebGpu— mirror.Max_12_Float_with_scalar_Nan_WebGpu— scalar NaN operand (element-wise scalar-operand path).Min_12_Float_Variadic_Nan_WebGpu— 3-input variadic with NaN in the middle operand (exercises the pairwise fold's NaN propagation through the intermediate).Max_12_Float16_Nan_WebGpu— f16 path (exercises the widen-then-bitcast branch).
Test #4 in particular is a genuinely non-trivial coverage add — it verifies that NaN survives one intermediate hop through the fold pipeline (input_0 op input_1 → intermediate_nan_carrier, then intermediate op input_2 → output_nan). If the WGSL select chain silently dropped NaN, that specific test would fail.
One minor consistency observation
The five new WebGPU tests are inconsistent about the WebGPU EP unavailable skip guard:
Max_12_Float_Nan_WebGpuhas it:auto webgpu_ep = DefaultWebGpuExecutionProvider(); if (!webgpu_ep) { GTEST_SKIP() << "WebGPU execution provider is not enabled in this build."; }
- The other four (
Min_12_Float_Nan_WebGpu,Max_12_Float_with_scalar_Nan_WebGpu,Min_12_Float_Variadic_Nan_WebGpu,Max_12_Float16_Nan_WebGpu) omit the skip and just pushDefaultWebGpuExecutionProvider()straight into the providers vector.
In a build without WebGPU EP, DefaultWebGpuExecutionProvider() returns nullptr. Pushing a null unique_ptr into the providers list would either crash or produce a misleading failure in test.Run(&execution_providers), depending on how the runner handles nulls. Sibling PRs (#29828 for HardSwish, #29830 for Reshape int64) consistently use the skip guard in every WebGPU-only test.
Recommend: either add the skip guard to the other four tests, or drop it from the first (if there's some conditional-compile guarantee I'm missing that makes it always non-null when this file is built). Consistency either way. If this test file is only compiled when the WebGPU EP is available (via a build gate I can't see from the diff), then the first test's guard is defensive-but-redundant and the pattern is fine as-is. Worth a one-liner reply from the author confirming which case applies.
Minor nits (not blockers)
- Macro naming:
WEBGPU_BINARY_VERSIONED_KERNEL(Max, 8, 11, Max, ...)is used to register a variadic operator (Maxinherits fromVariadicElementwiseviaWEBGPU_VARIADIC_IMPL). The macro name is a slight misnomer — it's actually a plain kernel-registration macro that doesn't care whether the op is binary or variadic. A follow-up rename toWEBGPU_ELEMENTWISE_KERNEL(or aWEBGPU_VARIADIC_KERNELalias) would tighten the naming. Fine for this PR. intermediate_tensors.reserve(input_count - 2)wheninput_count < 2would underflow. The N=1 case is early-returned; N=0 can't happen per operator schema. But a belt-and-suspendersif (input_count > 2) intermediate_tensors.reserve(input_count - 2)would be marginally safer against future refactors. Not worth pushing back on.- The outer broadcast-computation loop and the inner per-step broadcast are separately implemented (both use
ComputeBroadcastOutputShape). Consistent, but the outer loop'soutput_shapeisn't reused inside the fold — each fold step re-broadcasts. Small correctness comment: they compute different quantities (final multiway vs. running pairwise), so the redundancy is genuine, not accidental. - The comment
"The last fold writes directly to the kernel output."is a nice signposting for anyone tracing through the loop — good. WEBGPU_VARIADIC_IMPL(Max, "max_v(vec4<input_a_element_t>(a), vec4<input_b_element_t>(b))", GetMaxImpl)explicitly widens both operands viavec4<input_?_element_t>(...)— matches the patternEqualuses to make the WGSL builtin work in scalar-vs-vec4 broadcast mode. Correct.
CI note
46 / 86 checks OK on the head. First commit was 51/86 → 46/86 after the Copilot-fix commit aa0c37e. Small drop but not necessarily meaningful — could be flaky checks, retriggers, or pending legs still running. Since the Copilot fix was described as "Potential fix for pull request finding" (which I read as accepting an autofix suggestion), and I can't see the specific fix content, it's worth a scan of the failing legs to confirm the drop isn't real. Nothing in the code delta itself looks like it would break a previously-green leg.
Bottom line
Approve. The variadic-fold + NaN-bitcast + RunBinaryProgram refactor is a competent three-part change, each part well-motivated and correct. The x != x → integer-bitcast NaN detection is the kind of shader-compiler trap-avoidance that's worth documenting in the code (the author does, in the comment above GetMinMaxImpl). Test coverage hits the vec4-broadcast path, scalar-operand path, variadic-fold path, and the f16 widen-then-bitcast path — good spread.
Once the author confirms the WebGPU-EP-availability skip guard consistency (item #1 above) and CI settles, ready to merge.
|
@hariharans29 fixed the comments, please take another look. |
Re-review: PR #29833 — [WebGPU] Add Max and Min operators (head
|
| # | SHA | Message | CI |
|---|---|---|---|
| 1 | ac50ba2 |
[WebGPU] Add Max and Min operators | 51 / 86 |
| 2 | aa0c37e |
Potential fix for pull request finding (Verified) | 51 / 86 |
| 3 | 39477cb |
[WebGPU] Skip Max/Min NaN tests when the WebGPU EP is unavailable | pending |
| 4 | 55bf911 |
[WebGPU] Guard variadic elementwise reserve() against unsigned underflow | 1/1 so far |
Verdict: approve — both new commits address concerns cleanly. The pass-2 skip-guard inconsistency nit is fully resolved, and there's a bonus proactive fix for a latent underflow that I hadn't flagged.
Commit 3 (39477cb): Skip guards applied to remaining 4 tests
Directly addresses my pass-2 nit — the inconsistency where only Max_12_Float_Nan_WebGpu had the GTEST_SKIP() guard while the other four _WebGpu tests hard-push_back-ed the raw DefaultWebGpuExecutionProvider(). All four remaining tests (Min_12_Float_Nan_WebGpu, Max_12_Float_with_scalar_Nan_WebGpu, Min_12_Float_Variadic_Nan_WebGpu, Max_12_Float16_Nan_WebGpu) now use the same pattern:
auto webgpu_ep = DefaultWebGpuExecutionProvider();
if (!webgpu_ep) {
GTEST_SKIP() << "WebGPU execution provider is not enabled in this build.";
}
std::vector<std::unique_ptr<IExecutionProvider>> execution_providers;
execution_providers.push_back(std::move(webgpu_ep));
test.Run(...);All 5 WebGPU-specific NaN tests are now consistently guarded. This should also lift some of the 40+ CI-not-OK legs on aa0c37e, since WebGPU-disabled build configs will now skip cleanly instead of failing.
Commit 4 (55bf911): Defensive underflow guard on reserve()
The pre-existing code:
InlinedVector<Tensor> intermediate_tensors;
intermediate_tensors.reserve(static_cast<size_t>(input_count) - 2);Under the current control flow, input_count >= 2 is guaranteed by the earlier if (input_count == 1) return ...; early-out, so input_count - 2 >= 0 and the pre-existing code is correct. But if a future refactor moved or removed the early return, input_count == 1 would reach this line and static_cast<size_t>(1) - 2 would produce SIZE_MAX - 1, triggering a massive allocation attempt.
Fix:
// input_count >= 2 here (the single-input case returned above), so the last fold targets the
// kernel output and there are input_count - 2 intermediates. Guard the subtraction anyway so a
// future refactor can't turn it into an unsigned underflow.
if (input_count > 2) {
intermediate_tensors.reserve(static_cast<size_t>(input_count) - 2);
}The comment explicitly documents the current invariant (input_count ≥ 2) and states the defensive intent (future-proofing). Behavior is identical to the pre-existing code for all valid input_count values (reserve(0) is a no-op, so the new if input_count > 2 skip is semantically equivalent to the old reserve(0)). No functional regression.
Nice catch — I didn't flag this in pass-1 because the invariant was intact, but the defensive-coding rationale is fair. This is exactly the kind of latent trap that gets weaponized when someone later thinks "I can inline this helper" or "let me lift this early return into a caller."
Everything else from pass-2 stands
RunBinaryProgramextraction — clean.VariadicElementwisefold-pairwise withInlinedVectorpointer stability — correct.- NaN-propagation via integer bitcast (not
x != x) — right call vs. Tint/DXC fast-math. - fp16 widen-to-f32-before-bitcast — necessary due to vec4 byte-width.
- Integer types use plain builtin — no NaN possible.
- Registrations at opsets [8, 11] / [12, 12] / 13+ for both Max and Min — matches ONNX schema history.
- Provider table wiring — consistent.
Bottom line
Both new commits are correct and warranted. 39477cb closes the pass-2 test-consistency ask; 55bf911 is a proactive defense against a latent-but-currently-unreachable underflow. Approve. Watch CI on 55bf911 — should hopefully come back much greener than the earlier commits now that the 4 additional tests will skip cleanly on non-WebGPU legs.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Let me rebase to main for the CI. |
Implement the ONNX Max and Min operators natively in the WebGPU EP so models no longer fall back to the CPU EP for these ops. Implementation - Extract the two-input compute body of BinaryElementwise::ComputeInternal into a reusable RunBinaryProgram() helper. - Add a VariadicElementwise kernel that handles ONNX's variadic 1..N inputs with multidirectional (NumPy-style) broadcasting: 1 input copies to the output; N inputs are folded pairwise (acc = op(acc, input[i])) reusing the binary elementwise program. Intermediate results are held in a reserve()'d InlinedVector<Tensor> so pointers stay stable across RunProgram calls. - Register Max/Min for opsets 8-11, 12, and 13+ using WebGpuSupportedNumberTypes(), plus the kernel-create-info entries. - Normalize operands with vec4<input_*_element_t>(...) (as Equal does) so the WGSL max/min builtins work in vectorize-broadcast mode where one operand is scalar and the other is vec4. NaN propagation (ONNX opset 12+ requires it) - Float/float16 use a type-specific WGSL helper that detects NaN via an integer bitcast (exponent all ones, non-zero mantissa) instead of the x != x idiom, which the Tint/DXC fast-math path folds to false. - Integer types use the plain builtin (no NaN possible). Tests - Existing MathOpTest.Max*/Min* cases now also exercise the WebGPU EP for the supported types (float, float16, int32, uint32), covering broadcast and 3-input variadic shapes. - Add WebGPU-targeted NaN tests for Max/Min (elementwise, scalar-broadcast, variadic, and the float16 widen-then-bitcast path) to lock in NaN propagation on the WebGPU EP.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The five WebGPU-targeted Max/Min NaN tests pushed DefaultWebGpuExecutionProvider() into the providers vector without checking availability, so on a build without the WebGPU EP they would fail instead of skip. Add the standard availability guard to all five and move the checked EP into the vector so only one is created.
VariadicElementwise::ComputeInternal reaches the reserve() only with input_count >= 2 (the single-input case returns early), so reserve(input_count - 2) is safe today. Wrap it in `if (input_count > 2)` so a future refactor that removes the early return can't turn the size_t subtraction into an underflow / huge allocation.
The new WebGPU NaN tests built an OpTester and only then called GTEST_SKIP() when the WebGPU EP was unavailable. In Debug builds, ~BaseTester() raises SIGTRAP via DebugTrap() when Run() was never called, so these tests crashed onnxruntime_provider_test on every non-WebGPU Debug build (e.g. Android NNAPI). Move the EP-availability check above the OpTester construction so nothing is built when skipping.
|
Fix the CI failure by |
Description
Implement the ONNX Max and Min operators natively in the WebGPU EP so models no longer fall back to the CPU EP for these ops.
Implementation
NaN propagation (ONNX opset 12+ requires it)
Tests
Motivation and Context
See above.