Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

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

Open
stephentoub opened this issue Jul 6, 2023 · 7 comments
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Milestone

Comments

@stephentoub
Copy link
Member

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

@stephentoub stephentoub added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime.CompilerServices labels Jul 6, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 6, 2023
@ghost
Copy link

ghost commented Jul 6, 2023

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:

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

Author: stephentoub
Assignees: -
Labels:

api-suggestion, area-System.Runtime.CompilerServices

Milestone: -

@tannergooding
Copy link
Member

tannergooding commented Jul 6, 2023

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
Copy link
Contributor

"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
Copy link
Member

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
Copy link
Member

@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
Copy link
Member

jkotas commented Jul 14, 2023

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.

@tannergooding tannergooding added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Jul 14, 2023
@tannergooding tannergooding added this to the 9.0.0 milestone Jul 14, 2023
@terrajobst
Copy link
Member

terrajobst commented Jul 18, 2023

Video

  • Looks good as proposed
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);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 18, 2023
@stephentoub stephentoub self-assigned this Jul 18, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 19, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 6, 2023
@stephentoub stephentoub removed their assignment Nov 6, 2023
@stephentoub stephentoub modified the milestones: 9.0.0, Future Jul 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants