Allow explicitly implemented ISimdVector APIs to be handled as intrinsic #104983
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics |
tannergooding
requested review from
fanyang-mono,
BrzVlad,
kotlarmilos,
lambdageek,
steveisok and
MichalStrehovsky
as code owners
|
CC. @dotnet/jit-contrib, @fanyang-mono, @kg As per the title this fixes the handling so that the explicitly implemented It also ensures that the explicitly implemented ISimdVector APIs get the same handling as the regular intrinsic APIs to ensure that they don't pessimize the inlining budget and don't get unnecessarily viewed as generic when the types are well known. -- For Mono, there are still cases they'll hit the pessimized generic handling of course, but it should be more consistent now. |
| if (!strcmp (cmethod->name, "get_IsHardwareAccelerated")) { | ||
| const char* cmethod_name = cmethod->name; | ||
|
|
||
| if (strncmp(cmethod_name, "System.Runtime.Intrinsics.ISimdVector<System.Runtime.Intrinsics.Vector", 70) == 0) { |
There was a problem hiding this comment.
if memory serves, MonoMethod->name is just the method name, not the fullname, so these tests won't work. i think you would want to do mono_method_get_name_full, but that allocates so it would slow down interp code transforms to a probably unacceptable degree. looking at m_class_get_name(cmethod->klass) plus the method name might be enough. i'm not sure where the generic args go in the name, if anywhere.
There was a problem hiding this comment.
This is checking just the method name., the full name includes the prefix System.Runtime.Intrinsics.Vector128<T>.
For C#, explicitly implemented intrinsics have names in the format of FullNameOfInterface.Name. For example see: https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBLANgHwAEAmARgFgAoKgOwEMBbGAZwAc6wYACAWQE8AcoxbtOVAN5Uu0roQDMXbDQwwoAMw7cAkvy3LVGsZRldJxkzMIpeACgCUAbikyAvlWfT5s4lwDCXEC4dPj0VdU0PU0iTLytbO1MuN3MLLmjLa2DQg00AOh57ROSTZJcgA
Note that we have both:
.method public hidebysig
instance void M () cil managed
and
.method private final hidebysig newslot virtual
instance void MyNamespace.IMyInterface.M () cil managed
There was a problem hiding this comment.
-- so the method name is MyNamespace.IMyInterface.M and the full name of the method is MyNamespace.C::MyNamespace.IMyInterface.M (where I used :: to separate the class name from the method name)
There was a problem hiding this comment.
I see, so in this specific case due to how we implement the interface methods, the name resembles a fullname
|
I expect we can let this one can slip to .NET 10. Since ISimdVector is internal, the overall usage is limited. |
| int id = lookup_intrins (sri_vector_methods, sizeof (sri_vector_methods), cmethod); | ||
| const char *cmethod_name = cmethod->name; | ||
|
|
||
| if (strncmp(cmethod_name, "System.Runtime.Intrinsics.ISimdVector<System.Runtime.Intrinsics.Vector", 70) == 0) { |
There was a problem hiding this comment.
It would be nice to create a helper function to share the logic cross emit_sri_vector and emit_sri_vector_t.
There was a problem hiding this comment.
I agree, but I had initially viewed that as a larger/riskier refactoring this late in the .NET 9 cycle
Would you be fine with me logging an issue to track the cleanup for .NET 10? -- I think in practice we want S.Numerics and S.R.intrinsics to share the same importation helper and for any functionality to be differentiated based on the vector size. This is how we do it in RyuJIT since the behavior is typically functionally identical and the only real difference tends to be metadata passed into the IR nodes created (i.e. Vector4.operator + and Vector128.operator + are identical modulo the type handle; and Vector64/128/256/512<T> and Vector<T> are all the same modulo the vector size, etc).
| int id = lookup_intrins (sri_vector128_methods, sizeof (sri_vector128_methods), cmethod); | ||
| const char *cmethod_name = cmethod->name; | ||
|
|
||
| if (strncmp(cmethod_name, "System.Runtime.Intrinsics.ISimdVector<System.Runtime.Intrinsics.Vector", 70) == 0) { |
There was a problem hiding this comment.
It would be nice to create a helper function to share the logic.
|
hmmm, @fanyang-mono seeing some perf improvements, but its more like 1-5% in places; not what we were expecting and not to the level of resolving the regressions. For x64, there's the general issue of #105319 which is blocking most SIMD expansion. But for Arm64 there's still some cases that aren't being recognized and expanded, but I don't have enough context to dig into that and root cause it. It's possible that this is similar to the issue Egor was looking at for RyuJIT and is related to Generic Virtual Methods (#93174) and is places that Mono isn't able to see that it can be resolved to a concrete call and therefore can be expanded as an intrinsic; but its also possible its something else. |
|
@tannergooding Thanks for running the microbenchmarks. Let's go ahead and merge this, since there will be some improvements. And we could dig more later, when we have some bandwidth. |
This should improve the handling of generics constrained to
ISimdVectoron all runtimes and also ensures thatget_IsHardwareAcceleratedhas the correct behavior in R2R (otherwise it would return a different result from querying the concrete API).