KEMBAR78
Eliminate a bounds check in `NumberBuffer` by xtqqczze · Pull Request #81039 · dotnet/runtime · GitHub
Skip to content

Conversation

@xtqqczze
Copy link
Contributor

We check Debug.Assert(!digits.IsEmpty) in the ctor.

@ghost ghost added area-System.Numerics community-contribution Indicates that the PR has been added by a community member labels Jan 23, 2023
@ghost
Copy link

ghost commented Jan 23, 2023

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

Issue Details

We check Debug.Assert(!digits.IsEmpty) in the ctor.

Author: xtqqczze
Assignees: -
Labels:

area-System.Numerics

Milestone: -

public byte* GetDigitsPointer()
{
// This is safe to do since we are a ref struct
return (byte*)(Unsafe.AsPointer(ref Digits[0]));
Copy link
Member

Choose a reason for hiding this comment

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

We should ensure this doesn't impact inlining.

The IL is going to be larger as a result of the call and not every lowlevel API is currently Intrinsic and so may not "boost inlining" to account for the increase in IL size.

Copy link
Member

Choose a reason for hiding this comment

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

I believe MemoryMarshal.GetReference was recently updated to be Intrinsic and so should, but it would be good to double check that codegen isn't negatively impacted anywhere.

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 both old and new are under "always inline" limit so it's fine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added AggressiveInlining just to be sure.

Copy link
Member

@stephentoub stephentoub Jan 27, 2023

Choose a reason for hiding this comment

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

@tannergooding / @EgorBo, do we want the extra attribute?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe MemoryMarshal.GetReference was recently updated to be Intrinsic and so should, but it would be good to double check that codegen isn't negatively impacted anywhere.

There is no IntrinsicAttribute on MemoryMarshal.GetReference.

/// <summary>
/// Returns a reference to the 0th element of the Span. If the Span is empty, returns a reference to the location where the 0th element
/// would have been stored. Such a reference may or may not be null. It can be used for pinning but must never be dereferenced.
/// </summary>
public static ref T GetReference<T>(Span<T> span) => ref span._reference;

Copy link
Member

@tannergooding tannergooding Jan 27, 2023

Choose a reason for hiding this comment

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

do we want the extra attribute?

@stephentoub, RyuJIT currently defines ALWAYS_INLINE_SIZE = 16 and so any method with a number of IL bytes less than this should implicitly be treated this way. Other runtimes may have their own differing limits and, with MethodImplOptions being one of the metadata special attributes, there is no real harm in providing it since it doesn't impact IL metadata size. It effectively just ensures the JIT gives it the same multiplier.

That being said, SharpLab shows that GetDigitsPointer is 17 bytes of IL:

        IL_0000: ldarg.0
        IL_0001: ldfld valuetype [System.Runtime]System.Span`1<uint8> C::Digits
        IL_0006: call !!0& [System.Memory]System.Runtime.InteropServices.MemoryMarshal::GetReference<uint8>(valuetype [System.Runtime]System.Span`1<!!0>)
        IL_000b: call void* [System.Runtime]System.Runtime.CompilerServices.Unsafe::AsPointer<uint8>(!!0&)
        IL_0010: ret

Because of this, it is not guaranteed to be inlined even though what we're doing compiles down to a field read, so it would be directly beneficial to annotate it here.

@stephentoub stephentoub merged commit 59eb4df into dotnet:main Jan 30, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Numerics 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.

4 participants