Skip to content

Adds ccmp logic into emitter backend. - #112153

Merged
BruceForstall merged 3 commits into
dotnet:mainfrom
anthonycanino:apx-ccmp
Feb 15, 2025
Merged

Adds ccmp logic into emitter backend.#112153
BruceForstall merged 3 commits into
dotnet:mainfrom
anthonycanino:apx-ccmp

Conversation

@anthonycanino

@anthonycaninoanthonycanino commented Feb 4, 2025

Copy link
Copy Markdown
Contributor

Overview

This PR adds APX new ccmp instruction to the X86 backend.

Design

For reference, there is a unique extended evex encoding for ccmp:

image

where SC0 - SC3 encode the condition for ccmp to conditionally execute on (please see SDM Vol 1, Appendix B). If the status codes fail to satisfy the condition encoded by SC0 - SC3, no compare will be performed, and the OF, SF, ZF, and CF flags will be set to the default flag value (DFV) fields of, sf, zf and cf.

Testing

Note: The testing plan for APX work has been discussed in #106557, please refer to that PR for details, only results and comments will be posted in this PR. Results

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 4, 2025
@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Feb 4, 2025
@anthonycanino

anthonycanino commented Feb 4, 2025

Copy link
Copy Markdown
ContributorAuthor

1. Emitter unit tests

The left is output from JitDisasm, and right from JitLateDisasm.

image
image

2. Intel SDE testing

Test run with SDE:

base

3. SuperPMI results

Diffs are based on 2,623,457 contexts (1,043,127 MinOpts, 1,580,330 FullOpts).

MISSED contexts: 2,983 (0.11%)

Base JIT options: JitBypassApxCheck=1

Diff JIT options: JitBypassApxCheck=1

No diffs found.

Details

Context information

CollectionDiffed contextsMinOptsFullOptsMissed, baseMissed, diff
aspnet.run.windows.x64.checked.mch126,54063,09863,4422,665 (2.06%)2,665 (2.06%)
benchmarks.run.windows.x64.checked.mch28,757428,7530 (0.00%)0 (0.00%)
benchmarks.run_pgo.windows.x64.checked.mch105,61852,67952,9390 (0.00%)0 (0.00%)
benchmarks.run_tiered.windows.x64.checked.mch55,91238,40317,5090 (0.00%)0 (0.00%)
coreclr_tests.run.windows.x64.checked.mch582,221349,625232,5960 (0.00%)0 (0.00%)
libraries.crossgen2.windows.x64.checked.mch280,37716280,3610 (0.00%)0 (0.00%)
libraries.pmi.windows.x64.checked.mch295,0866295,0800 (0.00%)0 (0.00%)
libraries_tests.run.windows.x64.Release.mch751,895517,237234,6580 (0.00%)0 (0.00%)
libraries_tests_no_tiered_compilation.run.windows.x64.Release.mch342,81822,045320,7730 (0.00%)0 (0.00%)
realworld.run.windows.x64.checked.mch24,824224,8220 (0.00%)0 (0.00%)
smoke_tests.nativeaot.windows.x64.checked.mch29,4091229,397318 (1.07%)318 (1.07%)
2,623,4571,043,1271,580,3302,983 (0.11%)2,983 (0.11%)

@dotnet-policy-service

dotnet-policy-serviceBot commented Feb 4, 2025

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

@build-analysisbuild-analysisBot mentioned this pull request Feb 5, 2025
Open
3 tasks
@anthonycanino

anthonycanino commented Feb 10, 2025

Copy link
Copy Markdown
ContributorAuthor

I believe failures are related to #112163

@build-analysisbuild-analysisBot mentioned this pull request Feb 11, 2025
Open
3 tasks
@anthonycanino

anthonycanino commented Feb 12, 2025

Copy link
Copy Markdown
ContributorAuthor

@dotnet/jit-contrib could we get this looked at for a review to see if any changes are needed?

@JulieLeeMSFT

JulieLeeMSFT commented Feb 12, 2025

Copy link
Copy Markdown
Member

CC @amanasifkhalid and @EgorBo for code review.

@JulieLeeMSFTJulieLeeMSFT added this to the 10.0.0 milestone Feb 12, 2025
@EgorBo

EgorBo commented Feb 12, 2025•
edited
Loading

Copy link
Copy Markdown
Member

Do you have any idea why it shows up to 0.36% TP regression? Presumably, it's supposed to be zero-diff change?

@amanasifkhalid

amanasifkhalid commented Feb 13, 2025•
edited
Loading

Copy link
Copy Markdown
Contributor

Do you have any idea why it shows up to 0.36% TP regression? Presumably, it's supposed to be zero-diff change?

The insOpts enum now has entries that are larger than a byte -- perhaps the native compilers were previously compacting the enum (though that doesn't sound like a valid transformation)? Nothing else really sticks out to me.

@anthonycanino

anthonycanino commented Feb 13, 2025•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

There are a few places where SetDFVIfNeeded is called, particularly in emitInsRR that would have an additional if statement check. That's probably what it is.

We could adjust it so it only triggers for TARGET_AMD64 and not TARGET_X86. It currently follows the same precedent as SetNFIfNeeded etc.

BruceForstall
BruceForstall reviewed Feb 14, 2025
View reviewed changes

@BruceForstallBruceForstall left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to reduce the TP impact if possible. If this doesn't apply to x86, definitely make SetDFVIfNeeded() ifdef'ed for AMD64 (maybe create an empty function for x86 so call sites don't change). The full TP impact is only on MinOpts, fwiw.

Comment threadsrc/coreclr/jit/emit.h
_idCustom2 = ((value >> 1) & 1);
_idCustom3 = ((value >> 2) & 1);
_idCustom4 = ((value >> 3) & 1);
}

@BruceForstallBruceForstallFeb 14, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding:

assert(value == idGetEvexDFV());

Comment threadsrc/coreclr/jit/codegenxarch.cpp Outdated
emitter* theEmitter = GetEmitter();
genDefineTempLabel(genCreateTempLabel());

// #ifdef COMMENTOUT

@BruceForstallBruceForstallFeb 14, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

Comment threadsrc/coreclr/jit/codegenxarch.cpp Outdated
CORINFO_FIELD_HANDLE hnd = theEmitter->emitFltOrDblConst(1.0f, EA_4BYTE);
theEmitter->emitIns_R_C(INS_ccmpe, EA_4BYTE, REG_RAX, hnd, 0, INS_OPTS_EVEX_dfv_cf);
theEmitter->emitIns_R_C(INS_ccmpe, EA_4BYTE, REG_RAX, hnd, 4, INS_OPTS_EVEX_dfv_cf);
// #endif

@BruceForstallBruceForstallFeb 14, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

@anthonycanino

anthonycanino commented Feb 14, 2025

Copy link
Copy Markdown
ContributorAuthor

FYI throughput impact removed on x86 https://dev.azure.com/dnceng-public/public/_build/results?buildId=953002&view=ms.vss-build-web.run-extensions-tab

@BruceForstall
BruceForstall merged commit 01e4d44 into dotnet:mainFeb 15, 2025
@BruceForstallBruceForstall added the apx Related to the Intel Advanced Performance Extensions (APX) label Feb 15, 2025
grendello added a commit to grendello/runtime that referenced this pull request Feb 18, 2025
@grendello
Merge branch 'main' into dev/grendel/android-clr-host-local
e7e5f61
* main: (71 commits)
Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20250212.3 (dotnet#112626)
JIT: Unify struct arg morphing (dotnet#112612)
Enable `SA1015`: Closing generic bracket should not be followed by a space (dotnet#112597)
Clean up normalizeLocale for mono browser target (dotnet#112575)
SPMI: Ensure proper zero extension for isObjectImmutable and friends (dotnet#112617)
Quote --version-scripts path (dotnet#112603)
Remove incompatible API from PKCS netstandard2.0 lib
[main] Update dependencies from dotnet/emsdk (dotnet#112393)
Avoid `Unsafe.As` in `RangeCharSearchValues` (dotnet#112606)
Fixed the issue of incorrect return value of PalVirtualAlloc (dotnet#112579)
Fix size used for vectorization check in BitArray (dotnet#111558) (dotnet#111564)
Fix build of windows arm64 crossdac (dotnet#112553)
Simplify `ShuffleTakeIterator.GetCount` (dotnet#112593)
Fix VS div-by-0 in DacEnumerableHashTable code (dotnet#112542)
R2RDump: normalize GC info totalInterruptibleLength (dotnet#112003)
Fix alignment padding and add test for saving managed resources (dotnet#110915)
Adds `ccmp` logic into emitter backend. (dotnet#112153)
Disable AVX10.2 by default (dotnet#112572)
Outbox AesGcm in to Microsoft.Bcl.Cryptography
Make test `IUnknown` conforming (dotnet#112566)
...
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Mar 17, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@EgorBoEgorBoAwaiting requested review from EgorBo
@amanasifkhalidamanasifkhalidAwaiting requested review from amanasifkhalid
1 more reviewer
@BruceForstallBruceForstallBruceForstall approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@anthonycaninoanthonycanino

Labels

apxRelated to the Intel Advanced Performance Extensions (APX)area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

10.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@anthonycanino@JulieLeeMSFT@EgorBo@amanasifkhalid@BruceForstall