ISimdVector vector intrinsics fixes#113288
Conversation
ghost
added
the
lewing
force-pushed
the
test-sn-vector-interp
branch
from
1e58c80 to
13d6a60
Compare
lewing
force-pushed
the
test-sn-vector-interp
branch
from
13d6a60 to
d7bffcc
Compare
lewing
changed the title
lewing
force-pushed
the
test-sn-vector-interp
branch
from
ebcc1fb to
4f79607
Compare
lewing
force-pushed
the
test-sn-vector-interp
branch
3 times, most recently
from
934ec82 to
b6f4172
Compare
lewing
force-pushed
the
test-sn-vector-interp
branch
from
b6f4172 to
b3560ab
Compare
lewing
force-pushed
the
test-sn-vector-interp
branch
from
418452e to
ddb2b8f
Compare
done |
|
Tagging subscribers to this area: @BrzVlad, @kotlarmilos |
|
Tagging subscribers to this area: @steveisok, @vitek-karas |
|
Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics |
lewing
force-pushed
the
test-sn-vector-interp
branch
from
04516ad to
35bfdea
Compare
lewing
force-pushed
the
test-sn-vector-interp
branch
from
750fdc6 to
f318510
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the filtering for explicit ISimdVector implementations by addressing the incorrect namespace used for the Vector intrinsics. Key changes include adjusting the string slicing for processing method names and adding a new branch to handle the Runtime.Intrinsics.Vector namespace.
Comments suppressed due to low confidence (1)
src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs:4442
- Consider using an 'else if' here if the 'Numerics.Vector' branch is mutually exclusive, to prevent potential double processing of the method name.
if (partialMethodName.StartsWith("Runtime.Intrinsics.Vector"))
jeffhandley
left a comment
jeffhandley
left a comment
There was a problem hiding this comment.
Looks good to me and I agree with this capturing the original intent, but we should get signoff from a jit expert.
EgorBo
left a comment
EgorBo
left a comment
There was a problem hiding this comment.
LGTM. It's a bit unfortunate that we use magic constants here, any C++ compiler should be able to constant fold strlen("const"), but it's out of the scope of this PR
| (strncmp(cmethod_name + 70, "256<T>,T>.", 10) == 0) || | ||
| (strncmp(cmethod_name + 70, "512<T>,T>.", 10) == 0)) { | ||
| cmethod_name += 80; | ||
| if (strncmp(cmethod_name, "System.Runtime.Intrinsics.ISimdVector<System.", 45) == 0) { |
There was a problem hiding this comment.
Looks like we duplicate this code block 3 times, maybe put it into some shared function?
radekdoulik
left a comment
radekdoulik
left a comment
There was a problem hiding this comment.
Besides the code duplication it LGTM.
tannergooding
left a comment
tannergooding
left a comment
There was a problem hiding this comment.
LGTM
(Sorry for the delay, was out last week)
#104983 added filtering for explicit ISimdVector implementations. It appears it used the wrong namespace for
System.Numerics.Vector<T>when removing the expanded interface and types from the method name. Fix the explicit interface implementation filtering and call SN Vector interpreter intrinsics.