From 8c1dbfac40edaeef96e0a75a1e5bcfdc05c61cea Mon Sep 17 00:00:00 2001 From: Anton Firszov Date: Sun, 21 Oct 2018 23:38:12 +0200 Subject: [PATCH] address review findings + some more cleanup --- .../Helpers/SimdUtils.BasicIntrinsics256.cs | 28 ++++++++----------- .../Helpers/SimdUtils.ExtendedIntrinsics.cs | 14 +++------- .../SimdUtils.FallbackIntrinsics128.cs | 14 +++------- src/ImageSharp/Common/Helpers/SimdUtils.cs | 22 ++++++++++++++- src/ImageSharp/Common/Tuples/Octet.cs | 13 +++++++-- src/ImageSharp/Common/Tuples/Vector4Pair.cs | 10 +++---- .../JpegColorConverter.FromYCbCrSimd.cs | 2 +- .../JpegColorConverter.FromYCbCrSimdAvx2.cs | 2 +- .../ColorConverters/JpegColorConverter.cs | 4 +-- .../PixelFormats/PixelOperations{TPixel}.cs | 10 +++---- 10 files changed, 64 insertions(+), 55 deletions(-) diff --git a/src/ImageSharp/Common/Helpers/SimdUtils.BasicIntrinsics256.cs b/src/ImageSharp/Common/Helpers/SimdUtils.BasicIntrinsics256.cs index 713d606e7..0f1ce2ab6 100644 --- a/src/ImageSharp/Common/Helpers/SimdUtils.BasicIntrinsics256.cs +++ b/src/ImageSharp/Common/Helpers/SimdUtils.BasicIntrinsics256.cs @@ -28,7 +28,7 @@ namespace SixLabors.ImageSharp ref ReadOnlySpan source, ref Span dest) { - DebugGuard.IsTrue(source.Length == dest.Length, nameof(source), "Input spans must be of same size!"); + DebugGuard.IsTrue(source.Length == dest.Length, nameof(source), "Input spans must be of same length!"); if (!IsAvailable) { @@ -57,7 +57,7 @@ namespace SixLabors.ImageSharp ref ReadOnlySpan source, ref Span dest) { - DebugGuard.IsTrue(source.Length == dest.Length, nameof(source), "Input spans must be of same size!"); + DebugGuard.IsTrue(source.Length == dest.Length, nameof(source), "Input spans must be of same length!"); if (!IsAvailable) { @@ -78,16 +78,15 @@ namespace SixLabors.ImageSharp /// /// SIMD optimized implementation for . - /// Works only with `dest.Length` divisible by 8. + /// Works only with span Length divisible by 8. /// Implementation adapted from: /// http://lolengine.net/blog/2011/3/20/understanding-fast-float-integer-conversions /// http://stackoverflow.com/a/536278 /// internal static void BulkConvertByteToNormalizedFloat(ReadOnlySpan source, Span dest) { - GuardAvx2(nameof(BulkConvertByteToNormalizedFloat)); - - DebugGuard.IsTrue(ImageMaths.Modulo8(dest.Length) == 0, nameof(source), "dest.Length should be divisable by 8!"); + VerifyIsAvx2Compatible(nameof(BulkConvertByteToNormalizedFloat)); + VerifySpanInput(source, dest, 8); var bVec = new Vector(256.0f / 255.0f); var magicFloat = new Vector(32768.0f); @@ -128,9 +127,8 @@ namespace SixLabors.ImageSharp /// internal static void BulkConvertNormalizedFloatToByteClampOverflows(ReadOnlySpan source, Span dest) { - GuardAvx2(nameof(BulkConvertNormalizedFloatToByteClampOverflows)); - - DebugGuard.IsTrue(ImageMaths.Modulo8(source.Length) == 0, nameof(source), "source.Length should be divisible by 8!"); + VerifyIsAvx2Compatible(nameof(BulkConvertNormalizedFloatToByteClampOverflows)); + VerifySpanInput(source, dest, 8); if (source.Length == 0) { @@ -168,9 +166,9 @@ namespace SixLabors.ImageSharp } /// - /// Convert 'source.Length' values normalized into [0..1] from 'source' + /// Convert all values normalized into [0..1] from 'source' /// into 'dest' buffer of . The values are scaled up into [0-255] and rounded. - /// The implementation is SIMD optimized and works only with `source.Length` divisible by 8. + /// This implementation is SIMD optimized and works only when span Length is divisible by 8. /// Based on: /// /// http://lolengine.net/blog/2011/3/20/understanding-fast-float-integer-conversions @@ -178,12 +176,8 @@ namespace SixLabors.ImageSharp /// internal static void BulkConvertNormalizedFloatToByte(ReadOnlySpan source, Span dest) { - GuardAvx2(nameof(BulkConvertNormalizedFloatToByte)); - - DebugGuard.IsTrue( - ImageMaths.Modulo8(source.Length) == 0, - nameof(source), - "source.Length should be divisible by 8!"); + VerifyIsAvx2Compatible(nameof(BulkConvertNormalizedFloatToByte)); + VerifySpanInput(source, dest, 8); if (source.Length == 0) { diff --git a/src/ImageSharp/Common/Helpers/SimdUtils.ExtendedIntrinsics.cs b/src/ImageSharp/Common/Helpers/SimdUtils.ExtendedIntrinsics.cs index dfa6f189c..e0d6187dc 100644 --- a/src/ImageSharp/Common/Helpers/SimdUtils.ExtendedIntrinsics.cs +++ b/src/ImageSharp/Common/Helpers/SimdUtils.ExtendedIntrinsics.cs @@ -35,7 +35,7 @@ namespace SixLabors.ImageSharp ref ReadOnlySpan source, ref Span dest) { - DebugGuard.IsTrue(source.Length == dest.Length, nameof(source), "Input spans must be of same size!"); + DebugGuard.IsTrue(source.Length == dest.Length, nameof(source), "Input spans must be of same length!"); if (!IsAvailable) { @@ -62,7 +62,7 @@ namespace SixLabors.ImageSharp ref ReadOnlySpan source, ref Span dest) { - DebugGuard.IsTrue(source.Length == dest.Length, nameof(source), "Input spans must be of same size!"); + DebugGuard.IsTrue(source.Length == dest.Length, nameof(source), "Input spans must be of same length!"); if (!IsAvailable) { @@ -88,10 +88,7 @@ namespace SixLabors.ImageSharp /// internal static void BulkConvertByteToNormalizedFloat(ReadOnlySpan source, Span dest) { - DebugGuard.IsTrue( - ImageMaths.ModuloP2(dest.Length, Vector.Count) == 0, - nameof(source), - "dest.Length should be divisible by Vector.Count!"); + VerifySpanInput(source, dest, Vector.Count); int n = dest.Length / Vector.Count; @@ -126,10 +123,7 @@ namespace SixLabors.ImageSharp ReadOnlySpan source, Span dest) { - DebugGuard.IsTrue( - ImageMaths.ModuloP2(dest.Length, Vector.Count) == 0, - nameof(dest), - "dest.Length should be divisible by Vector.Count!"); + VerifySpanInput(source, dest, Vector.Count); int n = dest.Length / Vector.Count; diff --git a/src/ImageSharp/Common/Helpers/SimdUtils.FallbackIntrinsics128.cs b/src/ImageSharp/Common/Helpers/SimdUtils.FallbackIntrinsics128.cs index ab18a0067..565ea08f5 100644 --- a/src/ImageSharp/Common/Helpers/SimdUtils.FallbackIntrinsics128.cs +++ b/src/ImageSharp/Common/Helpers/SimdUtils.FallbackIntrinsics128.cs @@ -26,7 +26,7 @@ namespace SixLabors.ImageSharp ref ReadOnlySpan source, ref Span dest) { - DebugGuard.IsTrue(source.Length == dest.Length, nameof(source), "Input spans must be of same size!"); + DebugGuard.IsTrue(source.Length == dest.Length, nameof(source), "Input spans must be of same length!"); int remainder = ImageMaths.Modulo4(source.Length); int adjustedCount = source.Length - remainder; @@ -50,7 +50,7 @@ namespace SixLabors.ImageSharp ref ReadOnlySpan source, ref Span dest) { - DebugGuard.IsTrue(source.Length == dest.Length, nameof(source), "Input spans must be of same size!"); + DebugGuard.IsTrue(source.Length == dest.Length, nameof(source), "Input spans must be of same length!"); int remainder = ImageMaths.Modulo4(source.Length); int adjustedCount = source.Length - remainder; @@ -72,10 +72,7 @@ namespace SixLabors.ImageSharp [MethodImpl(InliningOptions.ColdPath)] internal static void BulkConvertByteToNormalizedFloat(ReadOnlySpan source, Span dest) { - DebugGuard.IsTrue( - ImageMaths.Modulo4(dest.Length) == 0, - nameof(dest), - "dest.Length should be divisible by 4!"); + VerifySpanInput(source, dest, 4); int count = dest.Length / 4; if (count == 0) @@ -109,10 +106,7 @@ namespace SixLabors.ImageSharp ReadOnlySpan source, Span dest) { - DebugGuard.IsTrue( - ImageMaths.Modulo4(source.Length) == 0, - nameof(source), - "source.Length should be divisible by 4!"); + VerifySpanInput(source, dest, 4); int count = source.Length / 4; if (count == 0) diff --git a/src/ImageSharp/Common/Helpers/SimdUtils.cs b/src/ImageSharp/Common/Helpers/SimdUtils.cs index 95a6030fd..fade8da79 100644 --- a/src/ImageSharp/Common/Helpers/SimdUtils.cs +++ b/src/ImageSharp/Common/Helpers/SimdUtils.cs @@ -154,12 +154,32 @@ namespace SixLabors.ImageSharp private static byte ConvertToByte(float f) => (byte)ImageMaths.Clamp((f * 255f) + 0.5f, 0, 255f); [Conditional("DEBUG")] - private static void GuardAvx2(string operation) + private static void VerifyIsAvx2Compatible(string operation) { if (!IsAvx2CompatibleArchitecture) { throw new NotSupportedException($"{operation} is supported only on AVX2 CPU!"); } } + + [Conditional("DEBUG")] + private static void VerifySpanInput(ReadOnlySpan source, Span dest, int shouldBeDivisibleBy) + { + DebugGuard.IsTrue(source.Length == dest.Length, nameof(source), "Input spans must be of same length!"); + DebugGuard.IsTrue( + ImageMaths.ModuloP2(dest.Length, shouldBeDivisibleBy) == 0, + nameof(source), + $"length should be divisable by {shouldBeDivisibleBy}!"); + } + + [Conditional("DEBUG")] + private static void VerifySpanInput(ReadOnlySpan source, Span dest, int shouldBeDivisibleBy) + { + DebugGuard.IsTrue(source.Length == dest.Length, nameof(source), "Input spans must be of same length!"); + DebugGuard.IsTrue( + ImageMaths.ModuloP2(dest.Length, shouldBeDivisibleBy) == 0, + nameof(source), + $"length should be divisable by {shouldBeDivisibleBy}!"); + } } } \ No newline at end of file diff --git a/src/ImageSharp/Common/Tuples/Octet.cs b/src/ImageSharp/Common/Tuples/Octet.cs index ae01a3121..539b74e32 100644 --- a/src/ImageSharp/Common/Tuples/Octet.cs +++ b/src/ImageSharp/Common/Tuples/Octet.cs @@ -3,8 +3,14 @@ using System.Runtime.InteropServices; namespace SixLabors.ImageSharp.Tuples { + /// + /// Contains 8 element value tuples of various types. + /// internal static class Octet { + /// + /// Value tuple of -s + /// [StructLayout(LayoutKind.Explicit, Size = 8 * sizeof(uint))] public struct OfUInt32 { @@ -34,7 +40,7 @@ namespace SixLabors.ImageSharp.Tuples public override string ToString() { - return $"[{this.V0},{this.V1},{this.V2},{this.V3},{this.V4},{this.V5},{this.V6},{this.V7}]"; + return $"{nameof(Octet)}.{nameof(OfUInt32)}({this.V0},{this.V1},{this.V2},{this.V3},{this.V4},{this.V5},{this.V6},{this.V7})"; } [MethodImpl(InliningOptions.ShortMethod)] @@ -51,6 +57,9 @@ namespace SixLabors.ImageSharp.Tuples } } + /// + /// Value tuple of -s + /// [StructLayout(LayoutKind.Explicit, Size = 8)] public struct OfByte { @@ -80,7 +89,7 @@ namespace SixLabors.ImageSharp.Tuples public override string ToString() { - return $"[{this.V0},{this.V1},{this.V2},{this.V3},{this.V4},{this.V5},{this.V6},{this.V7}]"; + return $"{nameof(Octet)}.{nameof(OfByte)}({this.V0},{this.V1},{this.V2},{this.V3},{this.V4},{this.V5},{this.V6},{this.V7})"; } [MethodImpl(InliningOptions.ShortMethod)] diff --git a/src/ImageSharp/Common/Tuples/Vector4Pair.cs b/src/ImageSharp/Common/Tuples/Vector4Pair.cs index 5988b2200..cae283d62 100644 --- a/src/ImageSharp/Common/Tuples/Vector4Pair.cs +++ b/src/ImageSharp/Common/Tuples/Vector4Pair.cs @@ -7,6 +7,7 @@ namespace SixLabors.ImageSharp.Tuples /// /// Its faster to process multiple Vector4-s together, so let's pair them! /// On AVX2 this pair should be convertible to of ! + /// TODO: Investigate defining this as union with an Octet.OfSingle type. /// [StructLayout(LayoutKind.Sequential)] internal struct Vector4Pair @@ -15,8 +16,6 @@ namespace SixLabors.ImageSharp.Tuples public Vector4 B; - private static readonly Vector4 Scale = new Vector4(1 / 255f); - [MethodImpl(MethodImplOptions.AggressiveInlining)] public void MultiplyInplace(float value) { @@ -52,8 +51,9 @@ namespace SixLabors.ImageSharp.Tuples b = b.FastRound(); // Downscale by 1/255 - this.A *= Scale; - this.B *= Scale; + var scale = new Vector4(1 / 255f); + this.A *= scale; + this.B *= scale; } /// @@ -74,7 +74,7 @@ namespace SixLabors.ImageSharp.Tuples public override string ToString() { - return $"{this.A}, {this.B}"; + return $"{nameof(Vector4Pair)}({this.A}, {this.B})"; } } } \ No newline at end of file diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.FromYCbCrSimd.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.FromYCbCrSimd.cs index 5c63a478d..1dc72aaf5 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.FromYCbCrSimd.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.FromYCbCrSimd.cs @@ -109,7 +109,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder.ColorConverters // Collect (r0,r1...r8) (g0,g1...g8) (b0,b1...b8) vector values in the expected (r0,g0,g1,1), (r1,g1,g2,1) ... order: ref Vector4Octet destination = ref Unsafe.Add(ref resultBase, i); - destination.Collect(ref r, ref g, ref b); + destination.Pack(ref r, ref g, ref b); } } } diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.FromYCbCrSimdAvx2.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.FromYCbCrSimdAvx2.cs index 3f26cdc90..46644258b 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.FromYCbCrSimdAvx2.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.FromYCbCrSimdAvx2.cs @@ -102,7 +102,7 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder.ColorConverters // Collect (r0,r1...r8) (g0,g1...g8) (b0,b1...b8) vector values in the expected (r0,g0,g1,1), (r1,g1,g2,1) ... order: ref Vector4Octet destination = ref Unsafe.Add(ref resultBase, i); - destination.Collect(ref rr, ref gg, ref bb); + destination.Pack(ref rr, ref gg, ref bb); } } } diff --git a/src/ImageSharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.cs b/src/ImageSharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.cs index 293f3bc1f..456636dc3 100644 --- a/src/ImageSharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.cs +++ b/src/ImageSharp/Formats/Jpeg/Components/Decoder/ColorConverters/JpegColorConverter.cs @@ -157,9 +157,9 @@ namespace SixLabors.ImageSharp.Formats.Jpeg.Components.Decoder.ColorConverters public Vector4 V0, V1, V2, V3, V4, V5, V6, V7; /// - /// Collect (r0,r1...r8) (g0,g1...g8) (b0,b1...b8) vector values in the expected (r0,g0,g1,1), (r1,g1,g2,1) ... order. + /// Pack (r0,r1...r7) (g0,g1...g7) (b0,b1...b7) vector values as (r0,g0,b0,1), (r1,g1,b1,1) ... /// - public void Collect(ref Vector4Pair r, ref Vector4Pair g, ref Vector4Pair b) + public void Pack(ref Vector4Pair r, ref Vector4Pair g, ref Vector4Pair b) { this.V0.X = r.A.X; this.V0.Y = g.A.X; diff --git a/src/ImageSharp/PixelFormats/PixelOperations{TPixel}.cs b/src/ImageSharp/PixelFormats/PixelOperations{TPixel}.cs index cbf164a71..6c133191a 100644 --- a/src/ImageSharp/PixelFormats/PixelOperations{TPixel}.cs +++ b/src/ImageSharp/PixelFormats/PixelOperations{TPixel}.cs @@ -30,11 +30,10 @@ namespace SixLabors.ImageSharp.PixelFormats internal virtual void PackFromVector4(ReadOnlySpan sourceVectors, Span destinationColors, int count) { ReadOnlySpan sourceVectors1 = sourceVectors; - Span destinationColors1 = destinationColors; - GuardSpans(sourceVectors1, nameof(sourceVectors1), destinationColors1, nameof(destinationColors1), count); + GuardSpans(sourceVectors1, nameof(sourceVectors1), destinationColors, nameof(destinationColors), count); ref Vector4 sourceRef = ref MemoryMarshal.GetReference(sourceVectors1); - ref TPixel destRef = ref MemoryMarshal.GetReference(destinationColors1); + ref TPixel destRef = ref MemoryMarshal.GetReference(destinationColors); for (int i = 0; i < count; i++) { @@ -53,11 +52,10 @@ namespace SixLabors.ImageSharp.PixelFormats internal virtual void ToVector4(ReadOnlySpan sourceColors, Span destinationVectors, int count) { ReadOnlySpan sourceColors1 = sourceColors; - Span destinationVectors1 = destinationVectors; - GuardSpans(sourceColors1, nameof(sourceColors1), destinationVectors1, nameof(destinationVectors1), count); + GuardSpans(sourceColors1, nameof(sourceColors1), destinationVectors, nameof(destinationVectors), count); ref TPixel sourceRef = ref MemoryMarshal.GetReference(sourceColors1); - ref Vector4 destRef = ref MemoryMarshal.GetReference(destinationVectors1); + ref Vector4 destRef = ref MemoryMarshal.GetReference(destinationVectors); for (int i = 0; i < count; i++) {