Add ILLink substitution for wasm intrinsics

[akoeplinger]

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)

[ghost]

Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Noticed those were missing while looking at #93399
Also deleted an empty file that was checked in by mistake.

Author:	akoeplinger
Assignees:	akoeplinger
Labels:	area-System.Runtime.Intrinsics
Milestone:	-

[akoeplinger]

@tannergooding looks like your recent PR broke the build :)

[MichalStrehovsky]

I wonder - do these do anything, or are we just cargo culting them since they were introduced in #37615?

The way we build CoreLib source code ensures that PackedSimd.IsSupported is hardcoded to return false on non-WASM (we build the PackedSimd.PlatformNotSupported.cs file):

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Wasm/PackedSimd.PlatformNotSupported.cs

Line 14 in 851c274

public static bool IsSupported { [Intrinsic] get { return false; } }

In the WASM CoreLib "supported" version of the file is supposed to recursively calls itself like these do for all the other platforms but it also returns false (I think this is a bug):

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Wasm/PackedSimd.cs

Line 17 in 851c274

public static bool IsSupported { [Intrinsic] get { return false; } }

ILLink should be able to inline the false value even without these substitutions (@vitek-karas @sbomer to double check me on this).

The reason why we do these as a recursive call on platforms where IsSupported could be true at runtime are twofold:

• Prevent ILLinker inlining the wrong value, like it probably does for PackedSimd on WASM right now
• Make sure reflection-invoking IsSupported does the right thing.

Sample of a correct IsSupported implementation here:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/AdvSimd.cs

Line 18 in 851c274

public static new bool IsSupported { get => IsSupported; }

Hardcoded to false on platforms that are not ARM:

runtime/src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/AdvSimd.PlatformNotSupported.cs

Line 22 in 851c274

public static new bool IsSupported { [Intrinsic] get { return false; } }

[akoeplinger]

In the WASM CoreLib "supported" version of the file is supposed to recursively calls itself like these do for all the other platforms but it also returns false (I think this is a bug):

I agree, I pushed a fix for that, let's see if it breaks anything 😄

ILLink should be able to inline the false value even without these substitutions

I don't see it doing that in practice though, if I remove the substitution then it doesn't inline the false value from the property.

[vitek-karas]

Could you please add a comment why it's done this way?
I was looking at this yesterday and it made no sense... :-)

Would be nice to have comments in the other places as well, at least eventually.

[akoeplinger]

There are quite a lot of intrinsics, I'll do that in a follow-up PR.

[jkotas]

[Intrinsic] means that the code you see may not be what is actually executed. The self-recursive implementation is used a lot with [Intrinsic] methods.

[vitek-karas]

ILLink should be able to inline the false value even without these substitutions (@vitek-karas @sbomer to double check me on this).

Yes - it should be able to do that. But it's definitely worth a test or validation (it depends on the callsite, if the condition around it is too complex we might give up on it).

[MichalStrehovsky]

I don't see it doing that in practice though, if I remove the substitution then it doesn't inline the false value from the property.

Looking at the source code, looks like ILLinker aborts this if there's IntrinsicAttribute on the method :(

[akoeplinger]

Is there a reason why we'd have [Intrinsic] on the non-supported case? maybe we should remove that?

[MichalStrehovsky]

Is there a reason why we'd have [Intrinsic] on the non-supported case? maybe we should remove that?

Yeah, that would probably help. I don't think it serves a purpose.

[jkotas]

Is there a reason why we'd have [Intrinsic] on the non-supported case? maybe we should remove that?

Yeah, that would probably help. I don't think it serves a purpose.

This was introduced in dotnet/coreclr#23751. It does serve purpose. It allows RyuJIT to get rid of the unreachable code sooner. If you delete [Intrinsic] on all unsupported cases, I expect that we will see perf regressions.

[tannergooding]

It does serve purpose.

What Jan said, but noting that it also provides a minor TP increase as the JIT doesn't have to process the IL, method body, etc. We can at import skip all the normal steps and just directly generate a node that returns constant false or which throws a PlatformNotSupportedException. This additionally allows dead code elimination and other low cost optimizations sooner.

[vitek-karas]

Looking at this in more detail - trimmer should really not assume a value of an Intrinsic method - since it can't know what the runtime will do. So I think the fact that it doesn't do constant propagation in this case is by design.

Which means we will need the substitution XML to tell it to do it anyway in these cases.

[akoeplinger]

Ok sounds like we're in agreement then that this is the way to go :) Merging.

[MichalStrehovsky]

It does serve purpose.

What Jan said, but noting that it also provides a minor TP increase as the JIT doesn't have to process the IL, method body, etc. We can at import skip all the normal steps and just directly generate a node that returns constant false or which throws a PlatformNotSupportedException. This additionally allows dead code elimination and other low cost optimizations sooner.

I don't see RyuJIT recognizing Wasm.PackedSimd. I don't think there's a single metric (JIT compile throughput, size on disk of the XML and IntrinsicAttribute, maintenance cost to maintain the XML, etc.) where keeping Wasm.PackedSimd.IsSupported marked Intrinsic wouldn't be causing a regression.

[jkotas]

I don't see RyuJIT recognizing Wasm.PackedSimd

RyuJIT is recognizing all IsSupported intrinsic methods under System.Runtime.Intrinsics namespace in CoreLib.

Compile Console.WriteLine(System.Runtime.Intrinsics.Wasm.PackedSimd.IsSupported) and run it with DOTNET_JitDump. You should see the IsSupported intrinsic recognized:

Importing BB01 (PC=000) of 'My:Main()'
    [ 0]   0 (0x000) call 0A000005
In Compiler::impImportCall: opcode is call, kind=0, callRetType is ubyte, structSize is 0
Named Intrinsic System.Runtime.Intrinsics.Wasm.PackedSimd.get_IsSupported: Unsupported - return false