Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use GetArrayDataReference in Vector* #104532

Closed
wants to merge 1 commit into from

Conversation

xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Jul 7, 2024

(System.Numerics.Vector`1[double]:.ctor(double[]):this)

-       mov      eax, dword ptr [rsi+0x08]
-       cmp      eax, 4
+       cmp      dword ptr [rsi+0x08], 4
        jl       SHORT G_M58252_IG04
        vmovups  ymm0, ymmword ptr [rsi+0x10]
        vmovups  ymmword ptr [rdi], ymm0
-						;; size=17 bbWeight=1 PerfScore 10.25
+						;; size=15 bbWeight=1 PerfScore 11.00

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 7, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 7, 2024
@MihaZupan
Copy link
Member

What is the intention behind the change? It looks like the JIT is already able to eliminate the bounds check in this case.

@MihaZupan MihaZupan added area-System.Runtime.Intrinsics and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 7, 2024
Copy link
Contributor

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

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jul 7, 2024

@MihuBot

@huoyaoyuan
Copy link
Member

This looks like a CQ issue for JIT. The semantic of the code doesn't change, with just less one register usage.
It could be tuned in JIT instead.

@jakobbotsch
Copy link
Member

I opened #104538 about improving the addressing mode recognition on the JIT side. It's definitely a deficiency we have seen before.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jul 8, 2024

@MihuBot

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jul 10, 2024

Bounds checks can be eliminated in some cases.

System.Runtime.Intrinsics.Vector128:Create[int](int[],int):System.Runtime.Intrinsics.Vector128`1[int]**

        test     edx, edx
        jl       SHORT G_M58180_IG04
        mov      eax, dword ptr [rsi+0x08]
-       mov      ecx, eax
-       sub      ecx, edx
-       cmp      ecx, 4
+       sub      eax, edx
+       cmp      eax, 4
        jl       SHORT G_M58180_IG04
-       cmp      edx, eax
-       jae      SHORT G_M58180_IG05
        mov      eax, edx
        vmovups  xmm0, xmmword ptr [rsi+4*rax+0x10]
        vmovups  xmmword ptr [rdi], xmm0
        mov      rax, rdi
-						;; size=35 bbWeight=1 PerfScore 12.75
+						;; size=29 bbWeight=1 PerfScore 11.25

@xtqqczze xtqqczze force-pushed the VectorGetArrayDataReference branch from 8a10bfd to a900207 Compare July 10, 2024 23:45
@xtqqczze xtqqczze marked this pull request as ready for review July 10, 2024 23:45
@xtqqczze
Copy link
Contributor Author

Updated PR with some eliminated bounds checks.

@xtqqczze
Copy link
Contributor Author

@MihuBot -arm64

@xtqqczze
Copy link
Contributor Author

@MihuBot

@xtqqczze
Copy link
Contributor Author

@MihuBot -arm64

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.Intrinsics community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants