Add support for AvxVnni instructions under Experimental. #51998

weilinwa · 2021-04-28T17:29:36Z

This is for #43780.

dotnet-issue-labeler · 2021-04-28T17:29:42Z

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 · 2021-04-28T17:29:44Z

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

Issue Details

This is for #43780.

Author:	weilinwa
Assignees:	-
Labels:	`area-System.Runtime.Intrinsics`, `new-api-needs-documentation`
Milestone:	-

tannergooding · 2021-04-28T17:39:54Z

CC. @echesakovMSFT for the HWIntrinsics side
@davidwrighton for the new ISA lightup

tannergooding · 2021-04-28T17:43:48Z

src/coreclr/jit/emitxarch.cpp

@@ -15391,6 +15401,10 @@ emitter::insExecutionCharacteristics emitter::getInsExecutionCharacteristics(ins
        case INS_vfnmsub132ss:
        case INS_vfnmsub213ss:
        case INS_vfnmsub231ss:
+        case INS_vpdpbusd:  //will be populated when the HW becomes publicly available


Can we get a small issue tracking this to ensure it doesn't get lost/forgotten?

Do you want me to open an issue now, or at the time when the code could be merged?

Either is fine, just as long as we are tracking updating it once the official numbers become available.

@tannergooding, #52121 is tracking this.

tannergooding · 2021-04-28T17:48:57Z

src/coreclr/jit/hwintrinsiccodegenxarch.cpp

+                                node->SetRegNum(op1Reg);
+                                targetReg = op1Reg;


We aren't doing this for any other HWIntrinsic instructions, instead we generate a movaps as required to put op1Reg in targetReg.

I'd expect register allocation to have already done everything at this point, but I'm not an expert here. @kunalspathak ?

@kunalspathak Does this change looks good to you or could you give me some suggestion on how should we handle it? Thanks.

I agree with @tannergooding . While I have limited knowledge, but I can see that we generate movaps if targetReg != op1Reg going forward in genHWIntrinsic_R_R_R_RM(). Is there a situation where that doesn't happen and we have to do targetReg = opt1Reg explicitely?

I double checked this part of the code and agree that we don't need to do this. I will update the code.

While I was checking this part, I encountered some issue when I set COMPlus_jitDump=RunBasicScenario_Load. The error message I saw is as the following (same error also found in other API such as the ones in Fma_vector256):

Assert failure(PID 5076 [0x000013d4], Thread: 26664 [0x6828]): Assertion failed 'type < CORINFO_TYPE_COUNT' in 'JIT.HardwareIntrinsics.X86.SimpleTernaryOpTest__MultiplyAddDouble:RunBasicScenario_Load():this' during 'Morph - Global' (IL size 137)

File: ...\runtime\src\coreclr\jit\ee_il_dll.hpp Line: 273 Image: ...\runtime\artifacts\bin\coreclr\windows.x64.Debug\corerun.exe

@tannergooding, do you know what is happening here?

I'd need to see the full stack trace, but I suspect this would be fixed by merging with (or rebasing onto) the latest dotnet/main.

For reference, there were a few fixes that went in (#52016, 0e12823, and #51627) resolving asserts that had cropped up related to adding native integer support to the HWIntrinsics code.

src/coreclr/vm/codeman.cpp

...raries/System.Private.CoreLib/src/System/Runtime/Intrinsics/X86/Avx2.PlatformNotSupported.cs

jkotas · 2021-04-28T18:10:16Z

Do we consider https://github.com/dotnet/designs/blob/main/accepted/2021/preview-features/preview-features.md to be approved?

Should we start using here instead of the *.Experimental package?

...ies/System.Runtime.Intrinsics.Experimental/ref/System.Runtime.Intrinsics.Experimental.csproj

tannergooding · 2021-04-28T18:11:59Z

Do we consider https://github.com/dotnet/designs/blob/main/accepted/2021/preview-features/preview-features.md to be approved?
Should we start using here instead of the *.Experimental package?

@terrajobst, what is your preference here? Do we want to ship these in S.P.Corelib with the new "preview" attribute or do we want to ship the Experimental package, or some combination of each?

src/tests/JIT/HardwareIntrinsics/X86/AvxVnni/MultiplyWideningAndAdd.Int16.cs

kunalspathak · 2021-04-30T19:07:09Z

src/coreclr/jit/hwintrinsiccodegenxarch.cpp

+                                node->SetRegNum(op1Reg);
+                                targetReg = op1Reg;


I agree with @tannergooding . While I have limited knowledge, but I can see that we generate movaps if targetReg != op1Reg going forward in genHWIntrinsic_R_R_R_RM(). Is there a situation where that doesn't happen and we have to do targetReg = opt1Reg explicitely?

terrajobst · 2021-05-06T23:24:32Z

@jkotas

Do we consider https://github.com/dotnet/designs/blob/main/accepted/2021/preview-features/preview-features.md to be approved?

Should we start using here instead of the *.Experimental package?

@tannergooding

@terrajobst, what is your preference here? Do we want to ship these in S.P.Corelib with the new "preview" attribute or do we want to ship the Experimental package, or some combination of each?

In the past, we have avoided changing API names between preview and RTM, we preferred packages. The reason being that having to change an API name guarantees a breaking change for everyone that used the feature, as opposed to only break the people who happen to depend on the parts that we changed based on feedback, which is usually only a fraction of all consumers. Using packages doesn't work well for cases where the API practically needs to live in existing assemblies or on existing types, which is precisely what the Preview design is meant to address.

So yes, I'm in favor of doing that instead of adding an Experimental namespace to corlib.

src/coreclr/jit/emitxarch.cpp

src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/X86/AvxVnni.cs

src/libraries/System.Runtime.Intrinsics.Experimental/System.Runtime.Intrinsics.Experimental.sln

tannergooding · 2021-05-13T15:43:55Z

...ies/System.Private.CoreLib/src/System/Runtime/Intrinsics/X86/AvxVnni.PlatformNotSupported.cs

+
+        /// <summary>
+        /// __m128i _mm_dpbusd_epi32 (__m128i src, __m128i a, __m128i b)
+        /// VPDPBUSD xmm, xmm, xmm


nit: Based on the JIT support, this (and the other APIs) should be:

Suggested change

/// VPDPBUSD xmm, xmm, xmm

/// VPDPBUSD xmm, xmm, xmm/m128

andusing ymm/m256 for the Vector256<T> APIs

tannergooding · 2021-05-13T15:45:49Z

@jkotas, @davidwrighton do the corinfoinstructionset changes require the JITEEVersionIdentifier to be updated as well?

The PR otherwise looks correct to me at this point.

jkotas · 2021-05-13T15:48:11Z

do the corinfoinstructionset changes require the JITEEVersionIdentifier to be updated as well?

Yes.

tannergooding · 2021-05-13T15:54:34Z

@weilinwa, as per Jan, the JITEEVersionIdentifier needs to be updated as part of this. That basically just involves updating this constant using uuidgen.exe -s (which should be available from the Visual Studio Developer Command Prompt): https://github.com/dotnet/runtime/blob/main/src/coreclr/inc/jiteeversionguid.h#L46-L51

For the current CI failures, it looks like the following changes are needed:

Download and apply the format.patch:: https://dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_apis/build/builds/1136469/artifacts?artifactName=format.windows.x64.patch&api-version=6.0&%24format=zip (simply extract this and run git apply <DownloadPath>\format.patch from your repo root, then commit
Add the [RequiresPreviewFeatures] attribute to the AvxVnni.PlatformNotSupported.cs file as well: https://github.com/dotnet/runtime/pull/51998/files#diff-e13e78cbf15eb79e74ebf837e0b5ad7009ed161af3e74afe2886ff355dd46e0cR9

weilinwa · 2021-05-20T17:33:15Z

@tannergooding, thanks for those very helpful info! I've maid changes accordingly. Could you take a look at the new changes?
There still are some CI failure shown here. Is there anything else I need to do?

tannergooding · 2021-05-20T23:41:25Z

@echesakovMSFT, @dotnet/jit-contrib this should have another pair of eyes before it is merged.

echesakov · 2021-05-21T00:08:17Z

@echesakovMSFT, @dotnet/jit-contrib this should have another pair of eyes before it is merged.

Sure, I will try to take a look tomorrow or earlier next week

imhameed · 2021-05-21T12:53:46Z

The Mono llvmaot lanes are still failing. :( Go ahead and disable the tests via issues.targets; I'll create an issue to follow up and fix this within Mono.

tannergooding · 2021-05-21T13:51:45Z

Thanks @imhameed. @weilinwa, disabling the tests should just require adding the following lines to this ItemGroup: https://github.com/dotnet/runtime/blob/main/src/tests/issues.targets#L1030

        <ExcludeList Include="$(XunitTestBinBase)/JIT/HardwareIntrinsics/X86/AvxVnni/*">
            <Issue>Mono crashes when new unsupported intrinsic groups are added, https://github.com/dotnet/runtime/issues/53078</Issue>
        </ExcludeList>

weilinwa · 2021-05-24T22:46:59Z

@tannergooding and @imhameed , I've added the ItemGroup to disable the tests but still see failures. Could you please take a look and let me know if I'm not disable them correctly or is there anything else causing the problem? Thanks.

imhameed · 2021-05-25T18:04:59Z

Use ** instead of *.

echesakov

LGTM

echesakov · 2021-05-26T16:11:57Z

src/coreclr/jit/hwintrinsiccodegenxarch.cpp

+                        case NI_AVXVNNI_MultiplyWideningAndAdd:
+                        case NI_AVXVNNI_MultiplyWideningAndAddSaturate:
+                        {
+                            assert(targetReg != REG_NA);


These assertions are not really needed here - we would check the same invariants at the beginning of genHWIntrinsic_R_R_R_RM

ghost · 2021-05-26T16:58:39Z

Hello @tannergooding!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

tannergooding · 2021-05-26T16:59:37Z

Thanks for all the work here @weilinwa. I've resolved the merge conflict in the JITEEVersionGUID and have marked this for auto-merge once CI passes.

We do still need a tracking issue for #51998 (comment)

weilinwa · 2021-05-26T17:24:17Z

Thanks @tannergooding and everybody for all the help. #52121 is the tracking issue we need.

weilinwa · 2021-06-01T22:07:01Z

@tannergooding, the CI still failed in some Mono related tests :( . Besides that, it also failed in the System.Security.Cryptography.Xml.Tests. Could you please help me to find solutions to these problems? Thanks!

tannergooding · 2021-06-01T22:43:12Z

I've requeued the failing jobs to see if they'll pass now. They looked like flaky tests so hopefully they pass this time.

If they do pass, that's great and if not we may be able to merge anyways if there are existing issues tracking them.

tannergooding · 2021-06-02T02:18:42Z

Merging, failures are:

Add support for AvxVnni instructions under Experimental.

7865cb8

dotnet-issue-labeler bot added area-System.Runtime.Intrinsics new-api-needs-documentation labels Apr 28, 2021

tannergooding reviewed Apr 28, 2021

View reviewed changes

src/coreclr/vm/codeman.cpp Show resolved Hide resolved

tannergooding reviewed Apr 28, 2021

View reviewed changes

...raries/System.Private.CoreLib/src/System/Runtime/Intrinsics/X86/Avx2.PlatformNotSupported.cs Outdated Show resolved Hide resolved

tannergooding reviewed Apr 28, 2021

View reviewed changes

...ies/System.Runtime.Intrinsics.Experimental/ref/System.Runtime.Intrinsics.Experimental.csproj Outdated Show resolved Hide resolved

weilinwa mentioned this pull request Apr 30, 2021

Add correct instruction execution characteristics for AVX-VNNI instrinsics #52121

Closed

kunalspathak reviewed Apr 30, 2021

View reviewed changes

tannergooding reviewed May 7, 2021

View reviewed changes

src/coreclr/jit/emitxarch.cpp Outdated Show resolved Hide resolved

tannergooding reviewed May 10, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/X86/AvxVnni.cs Show resolved Hide resolved

tannergooding reviewed May 10, 2021

View reviewed changes

src/libraries/System.Runtime.Intrinsics.Experimental/System.Runtime.Intrinsics.Experimental.sln Outdated Show resolved Hide resolved

weilinwa added 4 commits May 12, 2021 20:55

Add support for AvxVnni instructions

00a0a0b

Add preveiw feature attribute

83ef75d

Handle operands in lsra

12fd6bc

Undo changes for Experimental

31fd9cf

tannergooding reviewed May 13, 2021

View reviewed changes

weilinwa added 2 commits May 13, 2021 16:03

Update JITEEVersionIdentifier and fix remaining issues

35bdb57

Merge branch 'main' into experimental_hwintrinsic

9526655

weilinwa requested review from lambdageek, SamMonoRT and vargaz as code owners May 20, 2021 23:22

tannergooding approved these changes May 20, 2021

View reviewed changes

echesakov self-requested a review May 21, 2021 00:08

imhameed mentioned this pull request May 21, 2021

[mono] Don't crash when new unsupported intrinsic groups are added #53078

Closed

weilinwa added 2 commits May 21, 2021 17:12

Disable tests

e10d641

Disable Vector128 tests

297f855

Modify disable tests

eebd83b

echesakov approved these changes May 26, 2021

View reviewed changes

Merge branch 'main' into experimental_hwintrinsic

5d5f40e

tannergooding added the auto-merge label May 26, 2021

BruceForstall mentioned this pull request May 27, 2021

Rename superpmi tests collection names #53108

Merged

This was referenced May 27, 2021

FileSystemWatcher_DirectorySymbolicLink_TargetsFile_Fails #53366

Open

leakwheel failing in CI #53452

Closed

tannergooding merged commit ca889dd into dotnet:main Jun 2, 2021

runfoapp bot mentioned this pull request Jun 2, 2021

Host tests often fail on text file busy issues #53587

Closed

ghost locked as resolved and limited conversation to collaborators Jul 2, 2021

Add support for AvxVnni instructions under Experimental. #51998

Add support for AvxVnni instructions under Experimental. #51998

Conversation

weilinwa commented Apr 28, 2021

dotnet-issue-labeler bot commented Apr 28, 2021

ghost commented Apr 28, 2021

tannergooding commented Apr 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Apr 28, 2021

tannergooding commented Apr 28, 2021

Choose a reason for hiding this comment

terrajobst commented May 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tannergooding commented May 13, 2021

jkotas commented May 13, 2021

tannergooding commented May 13, 2021 • edited Loading

weilinwa commented May 20, 2021

tannergooding commented May 20, 2021

echesakov commented May 21, 2021

imhameed commented May 21, 2021

tannergooding commented May 21, 2021

weilinwa commented May 24, 2021

imhameed commented May 25, 2021

echesakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 26, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

tannergooding commented May 26, 2021

weilinwa commented May 26, 2021

weilinwa commented Jun 1, 2021

tannergooding commented Jun 1, 2021 • edited Loading

tannergooding commented Jun 2, 2021

tannergooding commented May 13, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

tannergooding commented Jun 1, 2021 •

edited

Loading