Use Unsafe.ReadUnaligned for vector creation in SpanHelpers.Fill
#111091
Conversation
ghost
added
the
dotnet-policy-service
bot
added
the
community-contribution
xtqqczze
force-pushed
the
use-unsafe-bitcast
branch
from
fe6f0ed to
1eb387c
Compare
xtqqczze
force-pushed
the
use-unsafe-bitcast
branch
from
1eb387c to
7ec2876
Compare
xtqqczze
force-pushed
the
use-unsafe-bitcast
branch
from
7ec2876 to
93e46f0
Compare
src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/SpanHelpers.T.cs
Outdated
Show resolved
Hide resolved
This reverts commit 93e46f0.
xtqqczze
changed the title
Unsafe.BitCast to avoid taking addressUnsafe.BitCast in SpanHelprs.Fill
xtqqczze
changed the title
Unsafe.BitCast in SpanHelprs.FillUnsafe.BitCast in SpanHelpers.Fill
xtqqczze
changed the title
Unsafe.BitCast in SpanHelpers.FillSpanHelpers.Fill
xtqqczze
changed the title
SpanHelpers.FillUnsafe.ReadUnaligned for vector creation in SpanHelpers.Fill
| if (sizeof(T) == 1) | ||
| { | ||
| vector = new Vector<byte>(Unsafe.As<T, byte>(ref tmp)); | ||
| vector = Vector.Create(Unsafe.As<T, byte>(ref tmp)); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
@xtqqczze could you please introduce such a helper (internal)? so it won't look like replacing one unsafe code with another
There was a problem hiding this comment.
we probably should have a centralized helper or some other API around...
Is there another place where this helper can be used? It looks very specific to implementation of this method.
it won't look like replacing one unsafe code with another
This is trying to replace unsafe code with more correct and less dangerous unsafe code. I think this should just use Unsafe.BitCast: #116221.
If we factor this out to a helper, the helper is going to be as unsafe as Unsafe.BitCast, so still unsafe.
|
Thanks for trying to improve this. Given the conversation in #111091 (comment), I'm going to close this, and it can be revisited when such a helper exists. |
|
Superseded by #116221. |
We should use
Unsafe.ReadUnalignedinstead ofUnsafe.As<T, ushort>(ref tmp)as the alignment of a typeTmay be less than its size.Zero code generation diffs.