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

Replace use of target dependent TestZ intrinsic #104488

Merged
merged 6 commits into from
Nov 27, 2024
Merged

Conversation

xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Jul 5, 2024

Contributes to #101251.

@xtqqczze xtqqczze changed the title Replace uses of TestZ intrinsic Replace use of target dependent TestZ intrinsic Jul 6, 2024
@jkotas jkotas 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 6, 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 xtqqczze marked this pull request as ready for review July 6, 2024 14:35
@xtqqczze
Copy link
Contributor Author

Depends on #102705.

@xtqqczze
Copy link
Contributor Author

@MihuBot

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jul 11, 2024

@tannergooding There are unexpected regressions, see MihuBot/runtime-utils#501

System.Text.Ascii:IsValidCore[ubyte](byref,int):ubyte

 G_M58774_IG07:
        cmp      esi, 64
        jg       SHORT G_M58774_IG08
        vmovups  ymm0, ymmword ptr [rdi]
-       vpor     ymm0, ymm0, ymmword ptr [rax-0x20]
-       vmovups  ymm1, ymmword ptr [reloc @RWD32]
-       vptest   ymm0, ymm1
+       vmovups  ymm1, ymmword ptr [rax-0x20]
+       vmovups  ymm2, ymmword ptr [reloc @RWD32]
+       vpternlogd ymm0, ymm1, ymm2, -88
+       vptest   ymm0, ymm0
        sete     cl
        movzx    rcx, cl
        jmp      SHORT G_M58774_IG04
-						;; size=35 bbWeight=0.50 PerfScore 10.75
+						;; size=42 bbWeight=0.50 PerfScore 12.00

@tannergooding
Copy link
Member

That particular case should be mostly resolved with #104517, vpternlog was missing general support for ensuring we select optimal containment

There's more improvements that could be had as well, but its a step in the right direction overall

@tannergooding
Copy link
Member

It's not really a regression per say and in other cases could be an optimization.

Either way, it shouldn't be impactful to this PR, I'll just finish the remaining work here and not fold in the case the AND is part of an op_Equality check

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jul 16, 2024

@tannergooding Depends on #104944.

@tannergooding
Copy link
Member

I wouldn't say this depends on #10944, I think it's fine to take as is. If you could mark it ready for review then I can give it a final pass and merge

@xtqqczze
Copy link
Contributor Author

@tannergooding I've suggested we make the suggested changes in a follow-up PR.

@tannergooding tannergooding merged commit cf7a744 into dotnet:main Nov 27, 2024
137 of 139 checks passed
mikelle-rogers pushed a commit to mikelle-rogers/runtime that referenced this pull request Dec 10, 2024
* Replace uses of `TestZ` intrinsic

* remove pragma warning disable IntrinsicsInSystemPrivateCoreLibConditionParsing

* refactor control flow

---------

Co-authored-by: Tanner Gooding <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Dec 28, 2024
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