Closed rickbrew closed 2 years ago
Hello! Thank you for bug report and for suggested solution!
To fix this bug is enough to change coefficient from 255.0f to 255.00001f (see the last commit).
Any way It would be great if you perform independend checking of this fix.
Okay I just tried it out: 1) changed coefficient from 255.0f to 255.00001f, 2) removed the Add(_epsFudge) (while also still excluding the Floor() call). It runs faster (~11%) and passes verification against known-good scalar path. Great!
There is on more change I made. The call to Min() should still use 255.0f, not 255.00001f, so two constants are needed. It did pass validation either way, so this may not actually be necessary in practice, and it did not affect my benchmark results.
Okay I think we can close this now :)
One last thing I'll add is that @saucecontrol came up with an even faster implementation of UnPremultiply, which I'll include here. It's in C# but should be straightforward to port back to C++ since there's a 1:1 mapping for all the intrinsics. Benchmarking shows this to be about 12% faster for AVX2, 18% faster for SSE4.1. It manages to use floating point only once per pixel group, for the division, and after that it's all integer. It also passes validation against a known-good scalar implementation.
// btw, nint is basically just ssize_t
public static void ConvertPbgra32ToBgra32Extent(uint* pExtent, nint length)
{
nint count = length;
byte* p = (byte*)pExtent;
if (Avx2.IsSupported && count >= Vector256<uint>.Count)
{
// Credit for this goes to Clinton Ingram, https://github.com/saucecontrol
var vmask0 = Vector256.Create(0x000000ff).AsByte();
var vmask1 = Vector256.Create(0x80808001, 0x80808005, 0x80808009, 0x8080800d, 0x80808001, 0x80808005, 0x80808009, 0x8080800d).AsByte();
var vmask2 = Vector256.Create(0x80808002, 0x80808006, 0x8080800a, 0x8080800e, 0x80808002, 0x80808006, 0x8080800a, 0x8080800e).AsByte();
var vmask3 = Vector256.Create(0x80808003, 0x80808007, 0x8080800b, 0x8080800f, 0x80808003, 0x80808007, 0x8080800b, 0x8080800f).AsByte();
var vmaskf = Vector256.Create(0, 4, 8, 12, 1, 5, 9, 13, 2, 6, 10, 14, 3, 7, 11, 15, 0, 4, 8, 12, 1, 5, 9, 13, 2, 6, 10, 14, 3, 7, 11, 15).AsByte();
var vscale = Vector256.Create((float)0xff00ff);
do
{
var vi = Avx.LoadVector256(p);
var vi3 = Avx2.Shuffle(vi, vmask3).AsInt32();
var vim = Avx.ConvertToVector256Int32WithTruncation(Avx.Divide(vscale, Avx.ConvertToVector256Single(vi3)));
vim = Avx2.BlendVariable(vim, Vector256<int>.Zero, Avx2.CompareEqual(vi3, Vector256<int>.Zero));
var vi2 = Avx2.Shuffle(vi, vmask2).AsInt32();
var vi1 = Avx2.Shuffle(vi, vmask1).AsInt32();
var vi0 = Avx2.And(vi, vmask0).AsInt32();
vi0 = Avx2.MultiplyLow(vi0, vim);
vi1 = Avx2.MultiplyLow(vi1, vim);
vi2 = Avx2.MultiplyLow(vi2, vim);
vi0 = Avx2.ShiftRightLogical(vi0, 16);
vi1 = Avx2.ShiftRightLogical(vi1, 16);
vi2 = Avx2.ShiftRightLogical(vi2, 16);
var vil = Avx2.PackSignedSaturate(vi0, vi1);
var vih = Avx2.PackSignedSaturate(vi2, vi3);
vi = Avx2.PackUnsignedSaturate(vil, vih);
Avx.Store(p, Avx2.Shuffle(vi, vmaskf));
p += Vector256<byte>.Count;
count -= Vector256<uint>.Count;
} while (count >= Vector256<uint>.Count);
}
if (Sse41.IsSupported && count >= 4)
{
// Credit for this goes to Clinton Ingram, https://github.com/saucecontrol
var vmask0 = Vector128.Create(0x000000ff).AsByte();
var vmask1 = Vector128.Create(0x80808001, 0x80808005, 0x80808009, 0x8080800d).AsByte();
var vmask2 = Vector128.Create(0x80808002, 0x80808006, 0x8080800a, 0x8080800e).AsByte();
var vmask3 = Vector128.Create(0x80808003, 0x80808007, 0x8080800b, 0x8080800f).AsByte();
var vmaskf = Vector128.Create(0, 4, 8, 12, 1, 5, 9, 13, 2, 6, 10, 14, 3, 7, 11, 15).AsByte();
var vscale = Vector128.Create((float)0xff00ff);
do
{
var vi = Sse2.LoadVector128(p);
var vi3 = Ssse3.Shuffle(vi, vmask3).AsInt32();
var vim = Sse2.ConvertToVector128Int32WithTruncation(Sse.Divide(vscale, Sse2.ConvertToVector128Single(vi3)));
vim = Sse41.BlendVariable(vim, Vector128<int>.Zero, Sse2.CompareEqual(vi3, Vector128<int>.Zero));
var vi2 = Ssse3.Shuffle(vi, vmask2).AsInt32();
var vi1 = Ssse3.Shuffle(vi, vmask1).AsInt32();
var vi0 = Sse2.And(vi, vmask0).AsInt32();
vi0 = Sse41.MultiplyLow(vi0, vim);
vi1 = Sse41.MultiplyLow(vi1, vim);
vi2 = Sse41.MultiplyLow(vi2, vim);
vi0 = Sse2.ShiftRightLogical(vi0, 16);
vi1 = Sse2.ShiftRightLogical(vi1, 16);
vi2 = Sse2.ShiftRightLogical(vi2, 16);
var vil = Sse2.PackSignedSaturate(vi0, vi1);
var vih = Sse2.PackSignedSaturate(vi2, vi3);
vi = Sse2.PackUnsignedSaturate(vil, vih);
Sse2.Store(p, Ssse3.Shuffle(vi, vmaskf));
p += Vector128<byte>.Count;
count -= Vector128<uint>.Count;
} while (count >= 4);
}
while (count > 0) { ... scalar implementation for the last few pixels ... }
}
@rickbrew looks like you missed the last update to my gist, which had one extra micro-optimization 😄
That's just swapping a blendvb
for a pandn
, which is same latency but smaller encoding (moreso on .NET since RyuJit emits 2 xorps
s for the two zero vector constants in the blend version).
Disassembly for the inner loop:
vmovdqu ymm6,ymmword ptr [rcx]
vpshufb ymm7,ymm6,ymm3
vcvtdq2ps ymm8,ymm7
vdivps ymm8,ymm5,ymm8
vcvttps2dq ymm8,ymm8
vxorps ymm9,ymm9,ymm9
vpcmpeqd ymm9,ymm7,ymm9
vpandn ymm8,ymm9,ymm8
vpshufb ymm9,ymm6,ymm2
vpshufb ymm10,ymm6,ymm1
vpand ymm6,ymm6,ymm0
vpmulld ymm6,ymm6,ymm8
vpmulld ymm10,ymm10,ymm8
vpmulld ymm9,ymm9,ymm8
vpsrld ymm6,ymm6,10h
vpsrld ymm10,ymm10,10h
vpsrld ymm9,ymm9,10h
vpackssdw ymm6,ymm6,ymm10
vpackssdw ymm7,ymm9,ymm7
vpackuswb ymm6,ymm6,ymm7
vpshufb ymm6,ymm6,ymm4
vmovdqu ymmword ptr [rcx],ymm6
@saucecontrol oh nice! I just confirmed correctness vs. known-good scalar code, and it's just a smidge faster than before :)
When unpremultiplying a value such as
#07070707
, the result should be#07FFFFFF
, but I'm getting#07FEFEFE
. There are several other instances of this, but always when A=B=G=R.Link to code in question: AVX2: https://github.com/ermig1979/Simd/blob/af41bdb76b69f6414aba4f40053393e91278a035/src/Simd/SimdAvx2AlphaBlending.cpp#L354 SSE4.1: https://github.com/ermig1979/Simd/blob/8ac6b68f7c9f91d16b34b13c6c860d8589c0f40a/src/Simd/SimdSse41AlphaBlending.cpp#L277
I found a fix, however. After the multiply, add a constant "fudge factor" of
0.00001f
. Also, the floor is not needed if convert-with-truncation is used (_mm256_cvttps_epi32
and_mm_cvttps_epi32
). So, the instruction count remains the same (I haven't yet benchmarked it though).Here is my working C# code, which should be easy enough to port back to C++: