Conversation
github-actions
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
Restores the AVX vhaddps / vhaddpd-based lowering for Vector256<float/double>.Sum() in the JIT to address a performance regression introduced by prior permute+add decomposition and lane-splitting, while maintaining deterministic lane-by-lane FP addition order.
Changes:
- Add an AVX-only fast path for 256-bit floating-point
Sum()usingNI_AVX_HorizontalAddand a final lane-combine step. - Keep the existing non-AVX floating-point path that sums 128-bit halves independently for determinism (software-fallback matching), but clarify the comment.
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
|
@EgorBot -amd using System;
using System.Numerics;
using System.Runtime.CompilerServices;
using System.Runtime.Intrinsics;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Jobs;
using BenchmarkDotNet.Running;
[MemoryDiagnoser]
public class Vector256RegressionBenchmark
{
private const float NullHeight = -3.4E+38f;
private const float FillTolerance = 0.01f;
private const float NegCutTolerance = -0.01f;
private float[] _baseElevations;
private float[] _topElevations;
[Params(1024, 4096)]
public int NumElevations { get; set; }
[GlobalSetup]
public void Setup()
{
var rng = new Random(42);
_baseElevations = new float[NumElevations];
_topElevations = new float[NumElevations];
for (var i = 0; i < NumElevations; i++)
{
_baseElevations[i] = rng.NextDouble() < 0.2
? NullHeight : (float)(rng.NextDouble() * 100);
_topElevations[i] = rng.NextDouble() < 0.2
? NullHeight : (float)(rng.NextDouble() * 100 + 10);
}
}
[Benchmark(Description = "Vector<float> portable SIMD", Baseline = true)]
public unsafe (double cut, double fill) PortableVector()
{
double cutVol = 0, fillVol = 0;
var nullVec = new Vector<float>(NullHeight);
var fillTolVec = new Vector<float>(FillTolerance);
var negCutTolVec = new Vector<float>(NegCutTolerance);
fixed (float* bp = _baseElevations, tp = _topElevations)
{
var bv = (Vector<float>*)bp;
var tv = (Vector<float>*)tp;
for (int i = 0, limit = NumElevations / Vector<float>.Count;
i < limit; i++, bv++, tv++)
{
var mask = ~(Vector.Equals(*bv, nullVec)
| Vector.Equals(*tv, nullVec));
if (Vector.Sum(mask) == 0) continue;
var delta = Vector.ConditionalSelect(mask, *tv, Vector<float>.Zero)
- Vector.ConditionalSelect(mask, *bv, Vector<float>.Zero);
var fillMask = Vector.GreaterThan(delta, fillTolVec);
var usedFill = -Vector.Sum(fillMask);
if (usedFill > 0)
fillVol -= Vector.Dot(delta, Vector.ConvertToSingle(fillMask));
if (usedFill < Vector<float>.Count)
{
var cutMask = Vector.LessThan(delta, negCutTolVec);
var usedCut = -Vector.Sum(cutMask);
if (usedCut > 0)
cutVol -= Vector.Dot(delta, Vector.ConvertToSingle(cutMask));
}
}
}
return (cutVol, fillVol);
}
[Benchmark(Description = "Vector256<float> explicit SIMD")]
public unsafe (double cut, double fill) ExplicitVector256()
{
if (!Vector256.IsHardwareAccelerated) return (0, 0);
double cutVol = 0, fillVol = 0;
var nullVec = Vector256.Create(NullHeight);
var fillTolVec = Vector256.Create(FillTolerance);
var negCutTolVec = Vector256.Create(NegCutTolerance);
fixed (float* bp = _baseElevations, tp = _topElevations)
{
var bv = (Vector256<float>*)bp;
var tv = (Vector256<float>*)tp;
for (int i = 0, limit = NumElevations / Vector256<float>.Count;
i < limit; i++, bv++, tv++)
{
var mask = ~(Vector256.Equals(*bv, nullVec)
| Vector256.Equals(*tv, nullVec));
if (Vector256.EqualsAll(mask, Vector256<float>.Zero)) continue;
var delta = Vector256.ConditionalSelect(mask, *tv, Vector256<float>.Zero)
- Vector256.ConditionalSelect(mask, *bv, Vector256<float>.Zero);
var fillMask = Vector256.GreaterThan(delta, fillTolVec);
if (Vector256.ExtractMostSignificantBits(fillMask) != 0)
fillVol += Vector256.Sum(
Vector256.ConditionalSelect(fillMask, delta, Vector256<float>.Zero));
var cutMask = Vector256.LessThan(delta, negCutTolVec);
if (Vector256.ExtractMostSignificantBits(cutMask) != 0)
cutVol += Vector256.Sum(
Vector256.ConditionalSelect(cutMask, delta, Vector256<float>.Zero));
}
}
return (cutVol, fillVol);
}
} |
src/coreclr/jit/gentree.cpp
Outdated
|
|
||
| if (simdSize == 32) | ||
| { | ||
| if (varTypeIsFloating(simdBaseType) && compOpportunisticallyDependsOn(InstructionSet_AVX)) |
There was a problem hiding this comment.
simdSize == 32 implies AVX support, so this check and the fallback code below can be removed.
| if (varTypeIsFloating(simdBaseType)) | ||
| { | ||
| // We need to ensure deterministic results which requires | ||
| // consistently adding values together. Since many operations | ||
| // end up operating on 128-bit lanes, we break sum the same way. | ||
| // Fallback for non-AVX: process lanes independently to ensure | ||
| // deterministic results matching the software fallback. | ||
|
|
There was a problem hiding this comment.
In the simdSize == 32 block, the if (varTypeIsFloating(simdBaseType)) at lines 27232–27240 is now unreachable because the floating-point case returns earlier (lines 27203–27225). This also makes the new comment about a "Fallback for non-AVX" misleading (simdSize==32 already implies AVX on xarch). Consider removing this dead branch or restructuring the logic as an else so the intent is clear.
|
Note This review was generated by Copilot. 🤖 Copilot Code Review — PR #126255Holistic AssessmentMotivation: Justified. The linked issue #126250 documents a 71% regression for Approach: Sound. The fix restores Summary: Detailed Findings✅ Correctness — Floating-point addition order is preservedTraced the addition order for both element types to verify determinism: float (8 elements in 256-bit):
double (4 elements in 256-bit):
Vector512 float/double recursively splits to two 256-bit halves and sums each via this path — the transitive improvement is correct. ✅ ISA guard — Assert is sufficientThe
|
Fixes #126250
Summary
Restore
vhaddps/vhaddpd-based lowering forVector256<float>.Sum()andVector256<double>.Sum()(and transitivelyVector512floating-point Sum), replacing the more expensivevpermilps + vaddpsdecomposition that was introduced in #95568 and later expanded in e012fd4.Root Cause
Two changes caused the regression:
vhaddpswith avpermilps + vaddps(shuffle + vertical add) sequence, claiming hadd was "not the most efficient."Vector256<float>.Sum()from 6 to 11 instructions.The determinism concern was valid in principle — the software fallback processes each 128-bit lane independently. However,
vhaddpsalready operates within 128-bit lanes (it is defined as two independent 128-bit horizontal adds), so it naturally preserves the same lane-by-lane addition order. The lane-splitting was unnecessary.Fix
For 256-bit floating-point Sum when AVX is available, use
NI_AVX_HorizontalAdd(vhaddps/vhaddpd) followed by a lane-combine step. This produces 4 instructions for float and 3 for double, vs the previous 11 and 5.Vector512floating-point Sum also benefits because it recursively calls the 256-bit Sum path.Codegen —
float Test(Vector256<float> vec) => Vector256.Sum(vec);Before (
vpermilps + vaddps, 62 bytes, 14 instructions)After (
vhaddps, 26 bytes, 7 instructions)58% less code, 50% fewer instructions. On AVX-512 hardware, the improvement is even larger because:
vhaddpshasHW_Flag_NoEvexSemantics, constraining registers toymm0–15and avoiding 4-byte EVEX prefixesValidation
Note
This comment was generated by Copilot.
Benchmark results with PR #126255
Benchmarked using the reporter's code on two AVX-512 server CPUs (cloud, via EgorBot):
Intel Emerald Rapids (Xeon 8573C)
AMD Turin (EPYC 9V45, Zen 5)
Summary
Consistent ~11% improvement on both Intel and AMD AVX-512 server CPUs. Tiger Lake (the reporter's CPU) is not available in the cloud; the improvement there would likely be larger due to the smaller µop cache being more sensitive to the 28% loop body size reduction.