Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix size used for vectorization check in BitArray (#111558) #111564

Merged
merged 2 commits into from
Feb 16, 2025

Conversation

AnsBalin
Copy link
Contributor

@AnsBalin AnsBalin commented Jan 18, 2025

This PR resolves #111558 by correcting the vector size used in the vectorization check within BitArray.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jan 18, 2025
@@ -885,7 +885,7 @@ public unsafe void CopyTo(Array array, int index)
}
}
}
else if (Ssse3.IsSupported && ((uint)m_length >= Vector512<byte>.Count * 2u))
else if (Ssse3.IsSupported && ((uint)m_length >= Vector128<byte>.Count * 2u))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think while you're on it, we can also replace various Sse2.And with just plain operators, e.g.

Vector128<byte> extractedLower = Sse2.And(shuffledLower, bitMask128);

to

Vector128<byte> extractedLower = shuffledLower & bitMask128;

but that's optional

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@jkotas
Copy link
Member

jkotas commented Jan 18, 2025

Run a quick perf test that shows this is a perf improvement for all affected sizes?

@MihaZupan
Copy link
Member

MihaZupan commented Jan 18, 2025

FWIW this would only affect pre-AVX2 X86 CPUs (or NAOT as I was informed we don't do Avx2 checks :/)

else if (Avx2.IsSupported && (uint)m_length >= Vector256<byte>.Count)
...
else if (Ssse3.IsSupported && ((uint)m_length >= Vector128<byte>.Count * 2u))
...
else if (AdvSimd.Arm64.IsSupported)

@AnsBalin
Copy link
Contributor Author

Seems like the test failures are all unrelated to this PR. Is there anything else I should do? @stephentoub

@eiriktsarpalis
Copy link
Member

Let's merge and retrigger to see what happens.

@AnsBalin
Copy link
Contributor Author

Let's merge and retrigger to see what happens.

@eiriktsarpalis I still do not think the remaining failures are related as they seem to be timeouts. Are they expected to pass? Let me know what else I can do! Thanks

@jkotas
Copy link
Member

jkotas commented Feb 16, 2025

@EgorBot -intel --envvars DOTNET_EnableAVX=0

using System.Collections;
using BenchmarkDotNet.Attributes;

public class Benchmarks
{
    bool[] buffer = new bool[1000];

    [Benchmark]
    [ArgumentsSource(nameof(TestData))]
    public void BitArrayCopyTo(BitArray bitArray) => bitArray.CopyTo(buffer, 0);

    public IEnumerable<BitArray> TestData()
    {
        yield return new BitArray(31);
        yield return new BitArray(32);
        yield return new BitArray(33);
        yield return new BitArray(64);
        yield return new BitArray(127);
        yield return new BitArray(128);
        yield return new BitArray(129);
        yield return new BitArray(256);
    }
}

@EgorBo
Copy link
Member

EgorBo commented Feb 16, 2025

@jkotas --envvars expects VAR:VALUE format (https://github.com/dotnet/BenchmarkDotNet/blob/master/docs/articles/guides/console-args.md#more) 🙂

@EgorBo
Copy link
Member

EgorBo commented Feb 16, 2025

@EgorBot -intel --envvars DOTNET_EnableAVX:0

using System.Collections;
using BenchmarkDotNet.Attributes;

public class Benchmarks
{
    bool[] buffer = new bool[1000];

    [Benchmark]
    [ArgumentsSource(nameof(TestData))]
    public void BitArrayCopyTo(BitArray bitArray) => bitArray.CopyTo(buffer, 0);

    public IEnumerable<BitArray> TestData()
    {
        yield return new BitArray(31);
        yield return new BitArray(32);
        yield return new BitArray(33);
        yield return new BitArray(64);
        yield return new BitArray(127);
        yield return new BitArray(128);
        yield return new BitArray(129);
        yield return new BitArray(256);
    }
}

@jkotas
Copy link
Member

jkotas commented Feb 16, 2025

Run a quick perf test that shows this is a perf improvement for all affected sizes?

This change improves the perf 3x-10x for the affected sizes (on machines without AVX): EgorBot/runtime-utils#296 (comment)

@jkotas
Copy link
Member

jkotas commented Feb 16, 2025

I still do not think the remaining failures are related

Right, the failures are unrelated.

@jkotas jkotas merged commit e334a1a into dotnet:main Feb 16, 2025
83 of 87 checks passed
grendello added a commit to grendello/runtime that referenced this pull request Feb 18, 2025
* main: (71 commits)
  Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250212.3 (dotnet#112626)
  JIT: Unify struct arg morphing (dotnet#112612)
  Enable `SA1015`: Closing generic bracket should not be followed by a space (dotnet#112597)
  Clean up normalizeLocale for mono browser target (dotnet#112575)
  SPMI: Ensure proper zero extension for isObjectImmutable and friends (dotnet#112617)
  Quote --version-scripts path (dotnet#112603)
  Remove incompatible API from PKCS netstandard2.0 lib
  [main] Update dependencies from dotnet/emsdk (dotnet#112393)
  Avoid `Unsafe.As` in `RangeCharSearchValues` (dotnet#112606)
  Fixed the issue of incorrect return value of PalVirtualAlloc (dotnet#112579)
  Fix size used for vectorization check in BitArray (dotnet#111558) (dotnet#111564)
  Fix build of windows arm64 crossdac (dotnet#112553)
  Simplify `ShuffleTakeIterator.GetCount` (dotnet#112593)
  Fix VS div-by-0 in DacEnumerableHashTable code (dotnet#112542)
  R2RDump: normalize GC info totalInterruptibleLength (dotnet#112003)
  Fix alignment padding and add test for saving managed resources (dotnet#110915)
  Adds `ccmp` logic into emitter backend. (dotnet#112153)
  Disable AVX10.2 by default (dotnet#112572)
  Outbox AesGcm in to Microsoft.Bcl.Cryptography
  Make test `IUnknown` conforming (dotnet#112566)
  ...
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
area-System.Collections community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong size used for vectorization check in BitArray
6 participants