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

Use Unsafe.ReadUnaligned for vector creation in SpanHelpers.Fill #111091

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Jan 4, 2025

We should use Unsafe.ReadUnaligned instead of Unsafe.As<T, ushort>(ref tmp) as the alignment of a type T may be less than its size.

Zero code generation diffs.

Copy link
Contributor

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

@xtqqczze xtqqczze force-pushed the use-unsafe-bitcast branch from 7ec2876 to 93e46f0 Compare January 4, 2025 23:12
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jan 5, 2025

@MihuBot

@xtqqczze xtqqczze changed the title Use Unsafe.BitCast to avoid taking address Use Unsafe.BitCast in SpanHelprs.Fill Jan 6, 2025
@xtqqczze xtqqczze changed the title Use Unsafe.BitCast in SpanHelprs.Fill Use Unsafe.BitCast in SpanHelpers.Fill Jan 9, 2025
@xtqqczze xtqqczze changed the title Use Unsafe.BitCast in SpanHelpers.Fill Refactor unsafe vector creation in SpanHelpers.Fill Feb 5, 2025
@xtqqczze xtqqczze marked this pull request as ready for review February 6, 2025 19:42
@xtqqczze xtqqczze changed the title Refactor unsafe vector creation in SpanHelpers.Fill Use Unsafe.ReadUnaligned for vector creation in SpanHelpers.Fill Feb 6, 2025
@@ -34,29 +34,29 @@ public static unsafe void Fill<T>(ref T refData, nuint numElements, T value)

if (sizeof(T) == 1)
{
vector = new Vector<byte>(Unsafe.As<T, byte>(ref tmp));
vector = Vector.Create(Unsafe.As<T, byte>(ref tmp));
Copy link
Member

Choose a reason for hiding this comment

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

these should rather use Vector.LoadUnsafe(ref tmp).AsByte() or similar.

We should be explicit that its a load API and not do extra or other things that the JIT has to elide or reason about

Copy link
Member

Choose a reason for hiding this comment

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

Ah nevermind, I see the issue here. This is doing a broadcast and is reinterpreting the ref to a supported T

This pattern in general is not great and something that we probably should have a centralized helper or some other API around...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Memory 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.

3 participants