Support float16 and bfloat16 in CPU ScatterElements reductions - #32025
Open
Alexander Novikov (novikov-alexander) wants to merge 2 commits into
Open
Support float16 and bfloat16 in CPU ScatterElements reductions#32025Alexander Novikov (novikov-alexander) wants to merge 2 commits into
Alexander Novikov (novikov-alexander) wants to merge 2 commits into
Conversation
ScatterElements with reduction='add' or 'mul' threw ORT_NOT_IMPLEMENTED on
the CPU EP for MLFloat16 and BFloat16, and 'min'/'max' threw for BFloat16.
docs/OperatorKernels.md already advertises tensor(float16) and
tensor(bfloat16) for this kernel, because the doc is generated from the
registrations and both types are in element_type_lists::All, so the op
claimed support it did not have.
The stubs exist because the generic functors use compound assignment and
neither half type defines operator+= or operator*=. Evaluate those two
reductions in float and round back to half on each update:
*a = MLFloat16(a->ToFloat() + b->ToFloat());
That is the same per-update rounding the CUDA and WebGPU kernels perform
(cuda/atomic/common.cuh, webgpu/tensor/scatter_elements.cc); neither carries
a float accumulator across updates, and the functor signature operates on
one element pair at a time, so matching them is also the natural fit.
Func_Min<BFloat16> and Func_Max<BFloat16> are removed rather than
implemented. Nothing forced those stubs: BFloat16 has the comparison
operators the generic functors need, so it now falls through to the generic
path exactly as MLFloat16 already did.
No registration or type constraint changes, so docs/OperatorKernels.md does
not need regenerating.
Tests: un-guards AddReduction_MLFloat16, which was compiled out behind
'#if defined(CUDA_VERSION)' with the comment that the operation is not
implemented on CPU, and adds fp16 mul and axis-1 add cases, bf16 add, mul,
min and max cases, and a case that pins the accumulation precision. The
last one accumulates eight 0.25 updates onto 1024.0; one ULP at 1024 is 1.0
in binary16, so per-update rounding leaves 1024 while a float accumulator
rounded once at the end would give 1026. All eight fail against the previous
kernel and pass with it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Two groups of the new tests assert things that are not properties of the operator, and would fail on providers that were never the subject. AddReduction_MLFloat16_RoundsAfterEachUpdate pins the accumulation precision. ONNX does not specify the intermediate precision, so rounding after every update is a property of this kernel; a provider that accumulated in float and rounded once would be equally valid and would produce 1026 instead of 1024. The bfloat16 tests would fail on the CUDA plugin EP for an unrelated reason: that kernel selects its compute type by element size, so it treats bfloat16 as float16 and computes 'add' and 'mul' on misread bits (microsoft#32061). Scoped conservatively, including min and max, since I have no CUDA hardware to confirm which of them survive the misinterpretation. Same mistake was found by CI on microsoft#32026, where a test asserting update ordering ran on every provider; fixing these before they get there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
|
Pushed ff3edae, scoping five of the new tests to the CPU EP before CI gets to them. Full CI has not run on this PR yet — it currently shows a single check — but it did run on #32026, and it failed there for exactly this reason: a test that asserted a property of the CPU kernel rather than of the operator was running on every provider. The same two patterns are present here, so I have fixed them rather than waiting.
The float16 I have applied the same scoping to the bfloat16 tests in #32065, which adds the equivalent support to |
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
ScatterElementswithreduction='add'or'mul'throwsORT_NOT_IMPLEMENTEDon the CPU EP forMLFloat16andBFloat16, and'min'/'max'throws forBFloat16. This implements them.The stubs exist for a mechanical reason: the generic functors use compound assignment (
(*a) += (*b)), and neither half type definesoperator+=oroperator*=. Soaddandmulare now evaluated in float and rounded back to half on each update:Func_Min<BFloat16>andFunc_Max<BFloat16>are removed rather than implemented. Nothing forced those two stubs —BFloat16already has the comparison operators the generic functors use, so it now falls through to the generic path exactly asMLFloat16has been doing all along (MinReduction_MLFloat16andMaxReduction_MLFloat16have been passing on the CPU EP unmodified).Accumulation precision: rounding to half after every update, not accumulating in float and rounding once. This matches the other backends that implement these reductions — CUDA (
cuda/atomic/common.cuh, nativeatomicAdd(half)on sm_70+ and a CAS loop that widens only for the single pairwise add below that) and WebGPU (webgpu/tensor/scatter_elements.cc,oldF16 + valuein f16). It is also the natural fit for a functor that receives one element pair at a time.No kernel registration or type-constraint changes, so
docs/OperatorKernels.mddoes not need regenerating.Motivation and Context
docs/OperatorKernels.mdalready liststensor(float16)andtensor(bfloat16)for CPUScatterElements, because the table is generated from the registrations and both types are inelement_type_lists::All. The op therefore advertised support it did not have: a float16 model usingreduction='add'fails at inference time withNOT_IMPLEMENTEDrather than falling back to anything.This is the shape most affected in practice — scatter-based pooling and embedding-style graphs that run in half precision end up needing a float32 cast around the scatter purely to work around the CPU kernel.
Verification
Built and ran locally on macOS/arm64:
onnxruntime_provider_test --gtest_filter="ScatterElements.*"— 18/18 pass (10 pre-existing, 8 new/enabled).scatter.cc, rebuilding, and re-running: exactly those 8 fail while the 10 pre-existing tests still pass. None of them pass vacuously.onnxruntime_provider_testsweep: 5644 tests, 5456 passed, 0 failures — unchanged from baseline. This includesGatherElements, which sharesScatterDataviaGatherElementsGradImpl.Tests
AddReduction_MLFloat16already existed but was compiled out behind#if defined(CUDA_VERSION)with the comment "Operation on float16 (MLFloat16) is not implemented on CPU". That guard is removed and the test now runs.Added: fp16
muland axis-1add; bf16add,mul,minandmax; andAddReduction_MLFloat16_RoundsAfterEachUpdate, which pins the accumulation semantics — it accumulates eight0.25updates onto1024.0, where one ULP is1.0in binary16, so per-update rounding leaves1024while a float accumulator rounded once at the end would produce1026. The gap is far outside the fp16 comparison tolerance, so the test genuinely discriminates between the two designs rather than merely passing.Note for reviewers: the bfloat16
mulexpectations use smaller factors than the float test, because bfloat16 keeps only 8 bits of significand and-343would round to-344.One thing worth flagging separately
While checking cross-backend behaviour I noticed the CUDA kernel dispatches
ScatterElementson element size (gather_elements.cc,GetElementType), which maps any 2-byte type toFLOAT16. CUDA therefore appears to compute bfloat16ScatterElementsas float16. That is out of scope here and I have not changed it, but with this PR the CPU and CUDA results for bfloat16 would differ. Happy to open a separate issue if that is useful.