Add X86Serialize hardware intrinsic. by anthonycanino · Pull Request #68677 · dotnet/runtime

anthonycanino · 2022-04-28T20:43:35Z

This PR implements #66467 for RyuJIT.

Some questions to address:

Do we need an X64 ISA for X86Serialize, since there are no X64 specific API/instructions?

ghost · 2022-04-28T20:43:40Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2022-04-28T20:43:46Z

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

Issue Details

This PR implements #66467 for RyuJIT.

Some questions to address:

Do we need an X64 ISA for X86Serialize, since there are no X64 specific API/instructions?

Author:	anthonycanino
Assignees:	-
Labels:	`area-CodeGen-coreclr`, `new-api-needs-documentation`
Milestone:	-

tannergooding · 2022-04-28T22:14:55Z

Do we need an X64 ISA for X86Serialize, since there are no X64 specific API/instructions?

Yes. This tracks the corresponding X86Serialize.X64.IsSupported check which while seemingly unnecessary actually allows versioning and correct API usage.

Effectively X86Serialize.X64.IsSupported implies X86Serialize.IsSupported && Environment.Is64BitProcess. This also implies any that base ISAs (just X86Base and X86Base.X64.IsSupported) are also true.

If we didn't expose this class and corresponding "ISA", then X86Serialize.X64.IsSupported would resolve to X86Base.X64.IsSupported and may report true when X86Serialize.IsSupported reports false, leading to UX issues.

anthonycanino · 2022-05-02T13:41:19Z

It looks like the failures are from a permission denied issue. I think that is related to #68715.

The superpmi failures are separate, but I believe that is expected as there are no mch files for a new JIT version id/interface?

Also, this PR implements the serialize instruction for RyuJIT. Is it possible to implement the intrinsic for mono in a follow up PR? @tannergooding @fanyang-mono

fanyang-mono · 2022-05-03T13:44:39Z

It looks like the failures are from a permission denied issue. I think that is related to #68715.

The superpmi failures are separate, but I believe that is expected as there are no mch files for a new JIT version id/interface?

Also, this PR implements the serialize instruction for RyuJIT. Is it possible to implement the intrinsic for mono in a follow up PR? @tannergooding @fanyang-mono

Feel free to create an issue to track the need of the Mono implementation of this, if you are not planing to implement it with this PR.

anthonycanino · 2022-05-03T20:27:48Z

@tannergooding do you have a recommendation for a perfscore for serialize? serialize will take as long as it takes to flush the instruction pipeline, i.e., its latency/throughput is dependent on the instructions in the pipeline. I can try to get a minimum latency/throughput, or we could try a worse case upper bound?

We've based our previous numbers based on the official timing numbers for Skylake (from Intel® 64 and IA-32 Architectures Software Developer Manuals iirc).

This is really just meant to be a rough heuristic and so an estimate or average case should be sufficient.

We are looking to get these numbers internally. For the time being, I have set both to be 1 cycle. Can we update these values once we have the numbers?

Yes, provided an issue is logged tracking it being updated.

Could we set it to something slightly more meaningful than 1 in the meantime? I'd expect this to at least be as expensive as a fencing operation (lfence, mfence, sfence).

I mirrored what was done for mfence, and set it to 50 cycles (more expensive than mfence, less expensive than cpuid which is ~100 cycles per the manual.

This seem ok? I will open up an issue now.

Seems great. Thanks!

anthonycanino · 2022-05-04T22:01:26Z

It looks like the failures are from a permission denied issue. I think that is related to #68715.
The superpmi failures are separate, but I believe that is expected as there are no mch files for a new JIT version id/interface?
Also, this PR implements the serialize instruction for RyuJIT. Is it possible to implement the intrinsic for mono in a follow up PR? @tannergooding @fanyang-mono

Feel free to create an issue to track the need of the Mono implementation of this, if you are not planing to implement it with this PR.

Will do. Thanks!.

anthonycanino · 2022-05-05T17:03:20Z

I've created the two issues as discussed and made the changes above. Can I get a full review now (as it was in draft until yesterday)?

tannergooding · 2022-05-09T16:43:23Z

CC. @dotnet/jit-contrib for second review.

tannergooding · 2022-05-10T20:12:58Z

Going to close and re-open this to restart CI against the latest HEAD. It's been a couple days but it also looks like the issues were unrelated infra issues that had cropped up.

tannergooding · 2022-05-11T15:38:37Z

@BruceForstall or @jakobbotsch, is SuperPMI expected to fail in builds with JIT/EE Version changes?

In particular, these are all reporting:

Using JIT/EE Version from mcs: f2a217c4-2a69-4308-99ce-8292c6763776

No Azure Storage MCH files to download from f2a217c4-2a69-4308-99ce-8292c6763776/linux/arm64/

[21:25:01] No MCH files found to replay

If so, this should be good to merge (just wanting to ensure I'm not missing skipping some subtle other issue here).

jakobbotsch · 2022-05-11T15:52:54Z

@BruceForstall or @jakobbotsch, is SuperPMI expected to fail in builds with JIT/EE Version changes?

Yes (but probably something we should improve). We should perhaps see if we can skip superpmi-diffs and superpmi-replay on JIT-EE GUID changes.

BruceForstall · 2022-05-11T16:37:08Z

superpmi-replay and superpmi-diffs already are attempting to avoid running on JIT-EE GUID changes, but apparently it's not doing what we hoped?

# Only run on changes to the JIT directory. Don't run if the JIT-EE GUID has changed,
# since there won't be any SuperPMI collections with the new GUID until the collection
# pipeline completes after this PR is merged.
pr:
  branches:
    include:
    - main
  paths:
    include:
    - src/coreclr/jit/*
    - src/coreclr/tools/superpmi/*
    exclude:
    - src/coreclr/inc/jiteeversionguid.h

tannergooding · 2022-05-11T17:08:24Z

Merging. Will log a tracking issue for the superpmi diffs running even when the JIT/EE version changes

tannergooding · 2022-05-11T17:08:48Z

Thanks for the contribution @anthonycanino!

tannergooding · 2022-05-11T17:11:19Z

Logged #69197

anthonycanino · 2022-05-11T20:10:45Z

Thanks for the reviews @tannergooding, @BruceForstall and @fanyang-mono !

anthonycanino requested review from MichalStrehovsky, SamMonoRT, lambdageek and vargaz as code owners April 28, 2022 20:43

ghost added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI new-api-needs-documentation labels Apr 28, 2022

ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 28, 2022

anthonycanino marked this pull request as draft April 28, 2022 20:43

anthonycanino force-pushed the serialize-api-impl branch from e4fb6f1 to 8475341 Compare April 28, 2022 23:30

runfoapp Bot mentioned this pull request Apr 29, 2022

RunContinueWithStressTestsNoState timing out in CI #2271

Closed

anthonycanino force-pushed the serialize-api-impl branch from 8475341 to f63abc2 Compare April 29, 2022 16:02

tannergooding reviewed Apr 29, 2022

View reviewed changes

Comment thread src/coreclr/jit/hwintrinsicxarch.cpp Outdated

tannergooding reviewed Apr 29, 2022

View reviewed changes

Comment thread src/coreclr/tools/Common/JitInterface/ThunkGenerator/InstructionSetDesc.txt Outdated

lambdageek requested a review from fanyang-mono April 29, 2022 16:41

anthonycanino force-pushed the serialize-api-impl branch from f63abc2 to b8d2afe Compare May 1, 2022 15:59

anthonycanino commented May 3, 2022

View reviewed changes

anthonycanino force-pushed the serialize-api-impl branch from b8d2afe to fb99562 Compare May 4, 2022 22:00

anthonycanino marked this pull request as ready for review May 4, 2022 22:01

anthonycanino force-pushed the serialize-api-impl branch 3 times, most recently from 9d97c75 to feffedd Compare May 5, 2022 16:46

This was referenced May 5, 2022

Add X86Serialize API and intrinsic for mono. #68916

Open

Update INS_serialize perfscore with accurate numbers. #68917

Closed

anthonycanino force-pushed the serialize-api-impl branch from feffedd to 4da87cf Compare May 5, 2022 17:01

anthonycanino force-pushed the serialize-api-impl branch from 4da87cf to c308def Compare May 9, 2022 13:12

Add X86Serialize hardware intrinsic.

2a3bfd5

anthonycanino force-pushed the serialize-api-impl branch from c308def to 2a3bfd5 Compare May 9, 2022 16:41

tannergooding approved these changes May 9, 2022

View reviewed changes

BruceForstall approved these changes May 10, 2022

View reviewed changes

tannergooding closed this May 10, 2022

tannergooding reopened this May 10, 2022

jakobbotsch approved these changes May 11, 2022

View reviewed changes

tannergooding merged commit dc7728f into dotnet:main May 11, 2022

tannergooding mentioned this pull request May 11, 2022

superpmi-diffs and superpmi-replay are running when jiteeversionguid.h changes #69197

Closed

tannergooding mentioned this pull request May 19, 2022

[API Proposal]: Expose xarch serialize instruction. #66467

Closed

JulieLeeMSFT mentioned this pull request Jun 3, 2022

What's new in .NET 7 Preview 5 [WIP] dotnet/core#7441

Closed

ghost locked as resolved and limited conversation to collaborators Jun 11, 2022

Navigation Menu

Uh oh!

Add X86Serialize hardware intrinsic. - #68677

Add X86Serialize hardware intrinsic.#68677
tannergooding merged 1 commit into
dotnet:mainfrom
anthonycanino:serialize-api-impl

anthonycanino commented Apr 28, 2022

ghost commented Apr 28, 2022

ghost commented Apr 28, 2022

tannergooding commented Apr 28, 2022

Uh oh!

Uh oh!

anthonycanino commented May 2, 2022

fanyang-mono commented May 3, 2022

anthonycanino May 3, 2022

tannergooding May 3, 2022

anthonycanino May 4, 2022

tannergooding May 5, 2022

anthonycanino May 5, 2022

tannergooding May 5, 2022

anthonycanino commented May 4, 2022

anthonycanino commented May 5, 2022

tannergooding commented May 9, 2022

tannergooding commented May 10, 2022

tannergooding commented May 11, 2022

jakobbotsch commented May 11, 2022

BruceForstall commented May 11, 2022

tannergooding commented May 11, 2022

tannergooding commented May 11, 2022

tannergooding commented May 11, 2022

anthonycanino commented May 11, 2022

Labels

5 participants

Uh oh!

Conversation

anthonycanino commented Apr 28, 2022

ghost commented Apr 28, 2022

ghost commented Apr 28, 2022

tannergooding commented Apr 28, 2022

Uh oh!

Uh oh!

anthonycanino commented May 2, 2022

fanyang-mono commented May 3, 2022

anthonycanino May 3, 2022

Choose a reason for hiding this comment

tannergooding May 3, 2022

Choose a reason for hiding this comment

anthonycanino May 4, 2022

Choose a reason for hiding this comment

tannergooding May 5, 2022

Choose a reason for hiding this comment

anthonycanino May 5, 2022

Choose a reason for hiding this comment

tannergooding May 5, 2022

Choose a reason for hiding this comment

anthonycanino commented May 4, 2022

anthonycanino commented May 5, 2022

tannergooding commented May 9, 2022

tannergooding commented May 10, 2022

tannergooding commented May 11, 2022

jakobbotsch commented May 11, 2022

BruceForstall commented May 11, 2022

tannergooding commented May 11, 2022

tannergooding commented May 11, 2022

tannergooding commented May 11, 2022

anthonycanino commented May 11, 2022

Labels

5 participants