Remove unsafe code from Guid.TryFormat #121652
Conversation
github-actions
bot
added
the
needs-area-label
EgorBo
added
area-System.Runtime
reduce-unsafe
and removed
needs-area-label
|
Tagging subscribers to this area: @dotnet/area-system-runtime |
| internal bool TryFormatCore<TChar>(Span<TChar> destination, out int charsWritten, int flags) | ||
| where TChar : unmanaged, IUtfChar<TChar> | ||
| { | ||
| Span<TChar> dest = destination; |
There was a problem hiding this comment.
Is there a tracking issue for the need to cache this in a local?
There was a problem hiding this comment.
Not sure, I didn't want to advertise it as a hack, it should be fixed by removing the old struct promotion eventually
There was a problem hiding this comment.
As far as "hacks" go, this one is probably one of the safest/simplest ones. Certainly better than using Unsafe.Add and friends.
I think it's better to explicitly track this so we know when we can remove it, so users can use it to avoid unsafe code themselves, and so users can know when they can remove their own workarounds.
| vecX.CopyTo(byteDest.Slice(0, Vector128<byte>.Count)); | ||
| vecX.CopyTo(byteDest.Slice(20, Vector128<byte>.Count)); | ||
| vecX.CopyTo(byteDest.Slice(8, Vector128<byte>.Count)); |
There was a problem hiding this comment.
What's the codegen for this currently? We've known to have issues for CopyTo in particular
Is one of the prior PRs where we tried to improve it going to be more applicable now?
| Span<TChar> dest = destination; | ||
|
|
||
| // For better range check elimination, split the dash and no-dash cases. | ||
| if (hasDashes) |
There was a problem hiding this comment.
There's a large amount of calls and casts in here.
I wonder if it's worth it compared to just having the 4 branch checks for hasDashes and manually avoiding the repeated operations
There was a problem hiding this comment.
the unified version is too complex for JIT to remove bounds checks for
There was a problem hiding this comment.
That's unfortunate. It would be nice to have a tracking issue for that, so we can know what patterns might be encouraging users (including ourselves) to write unsafe or less idiomatic code
| (Vector128<byte> vecX, Vector128<byte> vecY, Vector128<byte> vecZ) = FormatGuidVector128Utf8(this, flags < 0 /* dash */); | ||
| // Vectorized implementation for D, N, P and B formats: | ||
| // [{|(]dddddddd[-]dddd[-]dddd[-]dddd[-]dddddddddddd[}|)] | ||
| (Vector128<byte> vecX, Vector128<byte> vecY, Vector128<byte> vecZ) = FormatGuidVector128Utf8(this, hasDashes); |
There was a problem hiding this comment.
It's probably possible to make this fully xplat now and then we could remove the CompExactlyDependsOn
There was a problem hiding this comment.
probably, although, it relies on some helpers without fallbacks so will fail on e.g. wasm simd
2 participants
No description provided.