Conversation
Reduce each block of 32 vectors with the operator's vertical min/max and then search only the winning block, instead of carrying per-lane index vectors and blending them on every vector. Applies to IndexOfMin, IndexOfMax, IndexOfMinMagnitude and IndexOfMaxMagnitude for integer element types, for inputs of at least one full block.
Contributor
|
Tagging subscribers to this area: @dotnet/area-system-numerics |
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Author
|
Built the runtime locally and ran the suite on this branch — |
Author
|
You are right, thank you for the pointer — #133969 is the same change and it predates this one by five days. Closing in favour of it. For whatever it is worth to that PR, two things I ran into while doing this that may be worth checking there as well:
Sorry for the duplicate work. |
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.
Fixes #134045.
Why
IndexOfMinMaxCorekeeps a running best vector plus a vector of per-lane indices. Every vector costs a compare and two selects, plus the index increment, and the index vector has to fit the lane width, which is why theSize4Plus/Size2/Size1variants exist. The issue asked for the block search instead: reduce a block of vectors with the hardware min/max, then find the result inside the one block that won.How
For integer element types
IndexOfMinMaxCorenow runs a two-pass search:Compare. No index vectors and no blending.CompareplusIndexOfFirstMatch.Because pass 2 uses
!TOperator.Compare(best, x[i])rather than a bitwise match, the operator's own tie rule decides, so the result stays the first index, and the magnitude operators (where equal magnitudes prefer a sign) work unchanged. The vertical operation is the existingMinOperator/MaxOperator/MinMagnitudeOperator/MaxMagnitudeOperator, reached through three newMinMaxmembers onIIndexOfMinMaxOperator<T>, so each index operator keeps pairing with the same aggregation operator itsAggregatealready uses.Two restrictions, both measured:
float/doublekeep the existing implementation. The block loop would have to check for NaN per vector anyway (MinMaxCoredoes), and-0/+0and the first-NaN rule need care, so that is better as a separate change.32 * Vector512<T>.CountwhereVector512is accelerated, and so on). Below that the second pass costs more than it saves - at 8-64 elements the block search measured 0.6-0.8x of the current code - and gating on the widest width also keeps every input on at least as wide a vector as it uses today.Block bounds are computed as
blockStart + Math.Min(blockLength, x.Length - blockStart)so that they do not overflow for spans close toint.MaxValueelements.Test Plan
System.Numerics.Tensorstests pass: 5724 total, 0 failed (dotnet build /t:Test -c Release -f net11.0, osx-arm64).Added
IndexOfMin_FirstOfEqualMinimumsReturned,IndexOfMax_FirstOfEqualMaximumsReturned,IndexOfMinMagnitude_FirstOfEqualMagnitudesReturned,IndexOfMaxMagnitude_FirstOfEqualMagnitudesReturnedand an*AcrossBlocksvariant of each.Helpers.TensorLengthsonly goes to 256, which is below the threshold for most element types, so the*AcrossBlockstests useHelpers.TensorLengthsSpanningBlocks(511 to 4103) to cover the block boundaries, multi-block inputs, and a winner in the scalar tail. The existingIndexOf*_AllLengthstests accept any index holding an equal value, so nothing pinned the first-index rule that the block search has to preserve. Relaxing the block comparison toTOperator.Compare(blockBest, best) || blockBest == best(a plausible way to get this wrong) fails the new tests on 12 instantiations and nothing else.Differential check against a build of unmodified
main: 20,000 random cases per element type forint,uint,long,short,sbyte,byte,nint, with narrow value ranges (many duplicates), full ranges, and plantedMinValue/MaxValue, comparing all four APIs. Every result matches the current implementation.Benchmarks
Apple M-series, osx-arm64,
Vector128(no AVX2/AVX-512 here), release runtime, GElem/s, higher is better. Random data, best of 5 rounds.Below the threshold nothing changes, since those lengths keep taking the existing path (
intat N=8/16/32/64: 4.61/7.17/8.61/8.54 before, 4.65/7.28/8.99/9.19 after;byte: 2.02/6.44/9.33/11.63 before, 2.13/6.52/9.61/12.28 after).A second harness, written independently and alternating the two builds round by round with the median taken, puts the same cases slightly lower: 3.5-4.8x for
int/long/shortat 1024 and above, 4.9-7.3x forbyte, and 0.99-1.01x below the threshold. Right at the threshold the gain is smaller, since the second pass always rescans one block: 1.2-2.0x forintat 128, 1.6-2.2x forshortat 256.Two accumulators are worth keeping: with a single accumulator the same code reaches only 11.97 GElem/s for
intat N=65536 versus 21 with two.IndexOfMinMaxVectorsPerBlockis 32 because that is what the issue's own measurements favour (Blocks32there, withBlocks256no better); the worst case is one extra read of a single block, about 1.03x the reads.The three widths are written out separately, like the
IndexOfMinMaxVectorized*helpers they sit next to, becauseISimdVector<TSelf, T>is internal to CoreLib and not linked into this assembly.I do not have AVX-512 hardware, so the mask-register blending the issue describes is not what I measured here; the gain above comes from dropping the index vectors and the per-vector selects.