Skip to content

Use unaligned stores in WidenLatin1ToUtf16_Sse2 #135171

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.

Sign up for GitHub

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

Closed
EgorBo wants to merge 1 commit into from
Closed

Use unaligned stores in WidenLatin1ToUtf16_Sse2 #135171

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/Text/Latin1Utility.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1014,11 +1014,13 @@ private static unsafe void WidenLatin1ToUtf16_Sse2(byte* pLatin1Buffer, char* pU
// performance wins vs. other patterns. See for more information:
// https://github.com/dotnet/runtime/issues/33002

// pUtf16Buffer may be odd-aligned (e.g. MemoryMarshal.Cast<byte, char>), so it can't be

@jkotas jkotas Oct 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

MemoryMarshal.Cast<byte, char>

We expect byrefs and Spans to point to aligned memory. If your unsafe code creates misaligned byref or Span, you are on your own. BCL APIs are not guaranteed to work correct correctly with misaligned byrefs or Spans.

@jkotas jkotas Oct 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Added suggestion to dotnet/designs#375 to clarify this.

@EgorBo EgorBo Oct 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I generally agree, but the fix seemed harmless to me to make it slightly more durable at no cost?

@jkotas jkotas Oct 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The comment is wrong at minimum. It suggests that misaligned Spans are expected to work that is an invalid statement. This code is 100% correct as is.

If you believe that the change is harmless, I can live with it. I wish the instructions that throw on misaligned data were more available, so that we can use them everywhere the data are expected to be properly aligned.

@MihaZupan MihaZupan Oct 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

More problems with unaligned inputs called out in #84877 are still present in this file too, e.g.

runtime/src/libraries/System.Private.CoreLib/src/System/Text/Latin1Utility.cs

Line 237 in 6f1d933

pBuffer = (char*)(((nuint)pBuffer + SizeOfVector128InBytes) & ~(nuint)(SizeOfVector128InBytes - 1));

Should we look to fix those too if we're making changes like this?

@jkotas jkotas Oct 4, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's the problem with trying to make this better - you won't know where to stop and it won't ever be 100% right.

// aligned to 16 bytes by advancing whole chars. Use unaligned stores to stay correct.
Vector128<byte> low = Sse2.UnpackLow(latin1Vector, zeroVector);
Sse2.StoreAligned((byte*)pCurrentWriteAddress, low);
Sse2.Store((byte*)pCurrentWriteAddress, low);

Vector128<byte> high = Sse2.UnpackHigh(latin1Vector, zeroVector);
Sse2.StoreAligned((byte*)pCurrentWriteAddress + SizeOfVector128, high);
Sse2.Store((byte*)pCurrentWriteAddress + SizeOfVector128, high);

currentOffset += SizeOfVector128;
pCurrentWriteAddress += SizeOfVector128;
Expand Down
Loading