dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.25k stars 4.73k forks source link

[API Proposal]: Unsafe.IsAddressLessThanOrEqualTo / IsAddressGreaterThanOrEqualTo #88478

Open stephentoub opened 1 year ago

stephentoub commented 1 year ago

Background and motivation

When using Unsafe to write ref-based loops, it's common to want to iterate while one address is <= another. But there's no Unsafe.IsAddressLessThanOrEqualTo, so instead of being able to write:

do
{
    ...
}
while (Unsafe.IsAddressLessThanOrEqualTo(ref pos, ref oneVectorFromEnd));

you end up needing to do the mental gymnastics to come up with:

do
{
    ...
}
while (!Unsafe.IsAddressLessThan(ref oneVectorFromEnd, ref pos));

API Proposal

namespace System.Runtime.CompilerServices;

public static class Unsafe
{
    // Existing
    public static bool IsAddressGreaterThan<T>([AllowNull] ref T left, [AllowNull] ref T right);
    public static bool IsAddressLessThan<T>([AllowNull] ref T left, [AllowNull] ref T right);

    // New
+   public static bool IsAddressGreaterThanOrEqualTo<T>([AllowNull] ref T left, [AllowNull] ref T right);
+   public static bool IsAddressLessThanOrEqualTo<T>([AllowNull] ref T left, [AllowNull] ref T right);
}

API Usage

ref int oneVectorFromEnd = ref Unsafe.Subtract(ref end, Vector<int>.Count);
do
{
    current.StoreUnsafe(ref pos);
    current += increment;
    pos = ref Unsafe.Add(ref pos, Vector<int>.Count);
}
while (Unsafe.IsAddressLessThanOrEqualTo(ref pos, ref oneVectorFromEnd));

Alternative Designs

No response

Risks

No response

ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices See info in area-owners.md if you want to be subscribed.

Issue Details
### Background and motivation When using Unsafe to write ref-based loops, it's common to want to iterate while one address is <= another. But there's no Unsafe.IsAddressLessThanOrEqualTo, so instead of being able to write: ```C# do { ... } while (Unsafe.IsAddressLessThanOrEqualTo(ref pos, ref oneVectorFromEnd)); ``` you end up needing to do the mental gymnastics to come up with: ```C# do { ... } while (!Unsafe.IsAddressLessThan(ref oneVectorFromEnd, ref pos)); ``` ### API Proposal ```diff namespace System.Runtime.CompilerServices; public static class Unsafe { // Existing public static bool IsAddressGreaterThan([AllowNull] ref T left, [AllowNull] ref T right); public static bool IsAddressLessThan([AllowNull] ref T left, [AllowNull] ref T right); // New + public static bool IsAddressGreaterThanOrEqualTo([AllowNull] ref T left, [AllowNull] ref T right); + public static bool IsAddressLessThanOrEqualTo([AllowNull] ref T left, [AllowNull] ref T right); } ``` ### API Usage ```csharp ref int oneVectorFromEnd = ref Unsafe.Subtract(ref end, Vector.Count); do { current.StoreUnsafe(ref pos); current += increment; pos = ref Unsafe.Add(ref pos, Vector.Count); } while (Unsafe.IsAddressLessThanOrEqualTo(ref pos, ref oneVectorFromEnd)); ``` ### Alternative Designs _No response_ ### Risks _No response_
Author: stephentoub
Assignees: -
Labels: `api-suggestion`, `area-System.Runtime.CompilerServices`
Milestone: -
tannergooding commented 1 year ago

I think this is API is "goodness" from the perspective of making code that needs it easier to read/understand. It will make complex/dangerous code slightly more understandable.


That being said, byref manipulation is dangerous and very easy to get wrong. If you forget to create ref int oneVectorFromEnd and instead check against ref end, you likely have a GC hole. If you're doing reverse iteration and you decrement to one before the allocation, you have a GC hole, etc. -- NOTE: This isn't saying we shouldn't expose this API, just that I think we want to encourage users to use alternative patterns in most cases.

For vectorization, I think we really want to push users towards not manipulating the byref and using the overloads that take an index instead:

int remainder = input.Length % Vector<int>.Count;
int end = input.Length - remainder;

for (int i = 0; i < end; i += Vector<int>.Count)
{
    current.StoreUnsafe(ref pos, i);
    current += increment;
}

// Handle remainder

And if users really need to use byref manipulation, its slightly safer and more efficient to do this since you get to handle an extra iteration in the main loop:

// Given the below, where input.Length >= Vector<int>.Count:
//    ref int pos = MemoryMarshal.GetReference(input);
//    int remainder = input.Length % Vector<int>.Count;
//    ref int end = ref Unsafe.Add(ref pos, input.Length - remainder);

do
{
    Debug.Assert(Unsafe.IsAddressLessThan(ref pos, ref end));
    current.StoreUnsafe(ref pos);
    current += increment;
}
while (!Unsafe.AreSame(ref pos, ref end));

// Handle remainder
Sergio0694 commented 1 year ago

"I think we really want to push users towards not manipulating the byref and using the overloads that take an index instead"

For perf critical scenarios, wouldn't that be slightly less efficient? Last time we checked, the JIT wasn't doing loop strength reduction and still ended up doing one extra address computation per iteration, which you can sidestep entirely if you manually carry the shifting reference yourself instead 🤔


Following up from #85911, should we add a note that as soon as ref readonly params go online, these two APIs should be:

namespace System.Runtime.CompilerServices;

public static class Unsafe
{
    public static bool IsAddressGreaterThanOrEqualTo<T>([AllowNull] ref readonly T left, [AllowNull] ref readonly T right);
    public static bool IsAddressLessThanOrEqualTo<T>([AllowNull] ref readonly T left, [AllowNull] ref readonly T right);
}

?

tannergooding commented 1 year ago

Last time we checked, the JIT wasn't doing loop strength reduction and still ended up doing one extra address computation per iteration, which you can sidestep entirely if you manually carry the shifting reference yourself instead

Even in perf critical scenarios, one extra address computation isn't going to be typically meaningful in real world apps/scenarios. You're already going to be trading far more perf simply due to micro-architectural and hardware differences.

But more notably, we are frequently willing to trade a little bit of perf in favor of additional safety/maintainability of our code.

Longer term, we should work towards ensuring the JIT can do any relevant optimizations/transforms itself so that there isn't a difference.

tannergooding commented 1 year ago

@jkotas, any opinions on this API?

These would help readability and maintainability of code where devs deem it necessary to do the byref manipulation. But may also encourage users to continue writing the more problematic patterns that you've helped push us to avoid in the past, rather than pushing them more towards pinning or using the "safer" helpers (like LoadUnsafe(ref T, nuint index)) where possible.

jkotas commented 1 year ago

We have the full set of comparison methods or operators in other places (e.g. System.Numerics, System.Runtime.Intrinsics, System.Linq.Expressions, ...). I think it is fine to add the full set here for consistency as well even through it is just for convenience or style preference, and not strictly required.

I agree that we need to work towards simpler safe patterns for vectorization. There is a lot of mental gymnastics involved in manually vectorized code and unsafe byref arithmetic, adding these two APIs is not going to make a difference in either direction.

terrajobst commented 1 year ago

Video

namespace System.Runtime.CompilerServices;

public static class Unsafe
{
    // Existing
    // public static bool IsAddressGreaterThan<T>([AllowNull] ref T left, [AllowNull] ref T right);
    // public static bool IsAddressLessThan<T>([AllowNull] ref T left, [AllowNull] ref T right);

    // New
    public static bool IsAddressGreaterThanOrEqualTo<T>([AllowNull] ref T left, [AllowNull] ref T right);
    public static bool IsAddressLessThanOrEqualTo<T>([AllowNull] ref T left, [AllowNull] ref T right);
}