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

Add support for AvxVnni instructions under Experimental. #51998

Merged
merged 12 commits into from
Jun 2, 2021

Conversation

weilinwa
Copy link
Contributor

This is for #43780.

@dotnet-issue-labeler
Copy link

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
Copy link

ghost commented Apr 28, 2021

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
Copy link
Member

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tannergooding, #52121 is tracking this.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

Comment on lines 345 to 346
node->SetRegNum(op1Reg);
targetReg = op1Reg;
Copy link
Member

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

@jkotas
Copy link
Member

jkotas commented Apr 28, 2021

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
Copy link
Member

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?

Comment on lines 345 to 346
node->SetRegNum(op1Reg);
targetReg = op1Reg;
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

@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.


/// <summary>
/// __m128i _mm_dpbusd_epi32 (__m128i src, __m128i a, __m128i b)
/// VPDPBUSD xmm, xmm, xmm
Copy link
Member

Choose a reason for hiding this comment

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

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

Suggested change
/// VPDPBUSD xmm, xmm, xmm
/// VPDPBUSD xmm, xmm, xmm/m128

Copy link
Member

Choose a reason for hiding this comment

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

andusing ymm/m256 for the Vector256<T> APIs

@tannergooding
Copy link
Member

@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
Copy link
Member

jkotas commented May 13, 2021

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

Yes.

@tannergooding
Copy link
Member

tannergooding commented May 13, 2021

@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:

@weilinwa
Copy link
Contributor Author

@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
Copy link
Member

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

@echesakov
Copy link
Contributor

@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

@echesakov echesakov self-requested a review May 21, 2021 00:08
@imhameed
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Contributor

Use ** instead of *.

Copy link
Contributor

@echesakov echesakov left a comment

Choose a reason for hiding this comment

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

LGTM

case NI_AVXVNNI_MultiplyWideningAndAdd:
case NI_AVXVNNI_MultiplyWideningAndAddSaturate:
{
assert(targetReg != REG_NA);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@ghost
Copy link

ghost commented May 26, 2021

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
Copy link
Member

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
Copy link
Contributor Author

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

@weilinwa
Copy link
Contributor Author

weilinwa commented Jun 1, 2021

@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
Copy link
Member

tannergooding commented Jun 1, 2021

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 tannergooding merged commit ca889dd into dotnet:main Jun 2, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants