Do not ignore MemoryMarshal.TryWrite result in Guid#104728
Do not ignore MemoryMarshal.TryWrite result in Guid#104728xtqqczze wants to merge 3 commits intodotnet:mainfrom
MemoryMarshal.TryWrite result in Guid#104728Conversation
ghost
added
the
needs-area-label
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
dotnet-policy-service
bot
added
the
community-contribution
| if (BitConverter.IsLittleEndian) | ||
| { | ||
| MemoryMarshal.TryWrite(g, in this); | ||
| Unsafe.WriteUnaligned(ref MemoryMarshal.GetArrayDataReference(array), this); |
There was a problem hiding this comment.
does it make it any better? seems to be more verbose + bonus points for extra "Unsafe" word.
There was a problem hiding this comment.
@EgorBo updated description with a diff showing bounds check elimination
There was a problem hiding this comment.
I think it's better to extract the problematic part and file as an issue against JIT rather than workarounding it
There was a problem hiding this comment.
MemoryMarshal.TryWrite into a newly created new byte[16] shouldn't produce any checks, if it does - jit should be improved.
There was a problem hiding this comment.
The duplicate bounds check goes away with the addition of a uint cast here:
am11
added
area-System.Runtime.InteropServices
area-System.Runtime
and removed
needs-area-label
|
Tagging subscribers to this area: @dotnet/area-system-runtime |
This reverts commit 01675ac.
|
Reverted the |
| public bool TryWriteBytes(Span<byte> destination) | ||
| { | ||
| if (destination.Length < 16) | ||
| if ((uint)destination.Length < 16) |
There was a problem hiding this comment.
Jit definitely shouldn't need this, it should be aware that Span.Length is never negative
There was a problem hiding this comment.
There is an unsigned length check in MemoryMarshal.Write:
The JIT won't elide the check in the case where one length check is signed, the other unsigned:
There was a problem hiding this comment.
The check in MemoryMarshal.Write could be made signed, but then there could be a problem calling Write with a sliced span, due to the unsigned conditionals in Span.Slice.
There was a problem hiding this comment.
I don't see bound checks in your godbolt link, I do see two similar conditions that jit didn't remove - it's a different issue that we should also fix. Like I said previously, if JIT fails to fix an obvious case - we should file an issue instead of applying hacks in the codebase. These hacks still make sense for cases where JIT likely has no ability to know that certain external argument are never negative.
There was a problem hiding this comment.
I don't see bound checks
What I mean is that we currently have two different algorithms for bound checks specifically and a general redundant branch removal
There was a problem hiding this comment.
@AndyAyersMS you might be interested in the godbolt link above (https://csharp.godbolt.org/z/WTj89j5Mn) for RBO/JumpThreading missing opportunity
There was a problem hiding this comment.
There are similar conditions in Guid.TryWriteBytes and MemoryMarshal.Write, but no bounds checks per se .
There was a problem hiding this comment.
Btw, we have an issue to remove existing (uint) hacks - #67044 so we shouldn't do the opposite
There was a problem hiding this comment.
Btw, we have an issue to remove existing
(uint)hacks - #67044 so we shouldn't do the opposite
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
4 participants
No description provided.