[trimmer] per-RID linked corlib assembly contains foreign platform intrinsics classes #93399
Comments
ghost
added
the
|
Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics Issue DetailsThis happens on xamarin-android/main (.NET8), but I suppose it will apply to other platforms as well. I noticed that It appears some of the x86 classes are removed, but not all. For comparison, this is a list of types from the same assembly but linked for The Would it be possible to remove all of them? They are essentially dead weight for the target RID.
|
|
They're basically preserved because of this reference: This is coming from here: I'm assuming the linker isn't able to figure out that this |
|
I noticed the following after disassembling the linker binary: |
Noticed those were missing while looking at dotnet#93399
This is ReadyToRun-specific which doesn't apply to mono Contributes to dotnet#93399
Noticed those were missing while looking at #93399 Also deleted an empty file that was checked in by mistake. Fixed an issue where we weren't recursively calling IsSupported for PackedSimd which is the pattern we've been using (the JIT/AOT compiler is supposed to use the intrinsic instead)
This is ReadyToRun-specific which doesn't apply to mono, we can add Conditional logic so it is not compiled in Release mode. Also delete a duplicate file, we're only using `CompExactlyDependsOnAttribute.cs` Contributes to #93399
It's not clear why this would be needed. L941 is Correspondingly the inner So this seems rather like the trimmer is leaving code in for otherwise simple patterns, where there isn't really anything complex that should prevent it from understanding it can be kept alive or treated as dead |
|
@tannergooding I looked into this again and when building for arm64 the afaik these simd instructions are part of arm64 right? so we could add the substitutions for arm64. |
@akoeplinger, is Mono using different ILLink files than RyuJIT? We have https://github.com/tannergooding/runtime/blob/main/src/libraries/System.Private.CoreLib/src/ILLink/ILLink.Substitutions.NoArmIntrinsics.xml (with equivalents for x86/wasm) I know that there is then additional CoreCLR (https://github.com/dotnet/runtime/tree/main/src/coreclr/System.Private.CoreLib/src/ILLink) and Mono (https://github.com/dotnet/runtime/tree/main/src/mono/System.Private.CoreLib/src/ILLink) specific files, but I wouldn't expect those to conflict or cause the above to not be included |
|
right, that works for removing AdvSimd on x86/x64, but on arm64 we don't have anything that makes IsSupported=true |
|
Ah right and it can’t be constant true for the IL because the target platform might not support it. The JIT or AOT needs to be folding it to constant true So we would need to update this and other paths to be explicit about the negative isa check for the linker to be able to trim it |
|
That is, the linker isn’t smart enough to do the rewriting itself and understand that the else is dead because the outer if itself became |
for arm64 it could be constant true: runtime/src/mono/mono/mini/mini.c Lines 4543 to 4546 in c88c31b |
|
I don't think we want to rely on that, in case it changes in the future or Mono mirrors other core features that RyuJIT has (like supporting the I'd guess we also want the trimming to work for RyuJIT, so I think the best short term fix here is to identify places like the above and update them to explicitly have the checks the trimmer does support (that is ensure we do Long term, the "better" fix would be if the trimmer could identify cases like: if (boolExpr1 || boolExpr2)
{
if (boolExpr2)
{
}
else
{
}
}and understand that if |
dotnet-policy-service
bot
added
the
in-pr
|
Have a PR up here that should resolve most of this: #106777 Needs review if anyone has time |
@sbomer @jtschuster, @vitek-karas C = link-time constants
|
|
The problem is not that it's hard to figure out any specific case, the problem is that there are lot of cases to cover. The trimmer sees the IL, which look quite a bit less clean and to make things worse C# compiler will produce different IL between debug and release. The trimmer pattern matching code for this is already relatively complicated. That's actually why we intentionally written down the set of patterns which we support and we explicitly state that anything else is NOT supported (although it might work sometimes). We have test coverage for the supported patterns, so we can guarantee that behavior. I think what we're missing is additional diagnostics around this. I'll leave that up to the trimmer team, but one idea I had was to add logic into the analyzer (because it sees the C# which "looks nice") to detect supported cases, and for everything else it would produce a warning - this would only work for conditions which use the new feature switch attributed properties, since then the analyzer knows that it's a feature check and would produce the warning. It should flag the above cases as problematic. Obviously, we could expand the set of supported patterns, but I think we should be careful about doing that - the logic is currently implemented 3 times (Differently) so it's relatively costly to add anything new. |
|
I'm punting this out to 10.0.0. If folks feel strongly about backporting this to .NET 9 after a fix is merged, it can be reviewed and discussed. |
This happens on xamarin-android/main (.NET8), but I suppose it will apply to other platforms as well.
I noticed that
System.Private.CoreLib.dlloutput by the linker for arm64 RID contains X86 and WASM intrinsics types:It appears some of the x86 classes are removed, but not all. For comparison, this is a list of types from the same assembly but linked for
x84:The
x64version also contains some ARM intrinsics, but not all:Would it be possible to remove all of them? They are essentially dead weight for the target RID.
Attached is a zip with both assemblies mentioned above. The app being built was the
dotnet new androidtemplate for NET8corlib.zip
The text was updated successfully, but these errors were encountered: