Update a few code paths to ensure the trimmer can do its job #106777
Conversation
dotnet-issue-labeler
bot
added
the
needs-area-label
tannergooding
added
area-System.Runtime.Intrinsics
and removed
needs-area-label
|
@akoeplinger this should help resolve the trimmer size, is this a case you're wanting backported for .NET 9? I think for .NET 10 we probably want to do a more complete look at the places using hardware intrinsics in the BCL and ensure they're all doing straightforward checks that both R2R and the linker can trivially understand. |
tannergooding
force-pushed
the
fix-93399
branch
from
41c0a35 to
1d6ad1b
Compare
| internal static void ThrowUnreachableException() | ||
| { | ||
| #if NET | ||
| throw new UnreachableException(); |
There was a problem hiding this comment.
TIL that this exception type exists. Does it have any special meaning from the perspective of the trimmer?
There was a problem hiding this comment.
No, its just the exception type we added explicitly to represent paths that should be "unreachable", so its the most appropriate one to use here.
| { | ||
| // Workaround for missing MultiplyAddAdjacent on ARM | ||
| Vector128<short> even = AdvSimd.Arm64.TransposeEven(nibbles, Vector128<byte>.Zero).AsInt16(); | ||
| Vector128<short> odd = AdvSimd.Arm64.TransposeOdd(nibbles, Vector128<byte>.Zero).AsInt16(); | ||
| even = AdvSimd.ShiftLeftLogical(even, 4).AsInt16(); | ||
| output = AdvSimd.AddSaturate(even, odd).AsByte(); | ||
| } | ||
| else | ||
| { | ||
| // We explicitly recheck each IsSupported query to ensure that the trimmer can see which paths are live/dead |
There was a problem hiding this comment.
Could you explain why this added branch is necessary? My expectation is that the entire method should have been trimmed altogether if its preconditions are not met.
There was a problem hiding this comment.
Some of the checks are "dynamic" and not statically true/false.
For example, if (Sse2.IsSupported) is statically true for x86/x64 and if (AdvSimd.Arm64.IsSupported) is statically false, but if (Ssse3.IsSupported) is dynamic because it depends on the underlying hardware.
This means that on an x86/x64 system, the TryDecodeFromUtf16 will always be preserved in IL. Prior to this PR, the else path was therefore being kept for the case that we were on hardware where Ssse3.IsSupported was false and that rooted AdvSimd.Arm64 on xarch unnecessarily.
With this change, we instead now allow the AdvSimd path to be trimmed out as dead code and the IL will instead keep the ThrowUnreachableException which allows things like R2R, the JIT, or AOT compilers to trim it out as dead code (because they will know the target hardware and treat Ssse3.IsSupported as constant, then allowing the method to be removed).
This should help resolve #93399
There are potentially other less obvious cases I missed and there's quite a bit of code that is relying on the
[CompExactlyDependsOn(...)]attribute and a higher level check for the trimmer to "work" as expected.