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

Adding the Vector512 and Vector512<T> types #76642

Merged
merged 39 commits into from
Jan 12, 2023

Conversation

tannergooding
Copy link
Member

@tannergooding tannergooding commented Oct 4, 2022

This resolves #73262
This resolves #75791

@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 ghost assigned tannergooding Oct 4, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Oct 4, 2022

Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics
See info in area-owners.md if you want to be subscribed.

Issue Details

Marking this as draft since we need to add corresponding tests to https://github.com/dotnet/runtime/tree/main/src/tests/JIT/HardwareIntrinsics/General, but it is pending #74886 to avoid conflicts.

Author: tannergooding
Assignees: tannergooding
Labels:

area-System.Runtime.Intrinsics, new-api-needs-documentation

Milestone: -

@vargaz
Copy link
Contributor

vargaz commented Oct 26, 2022

The mono changes look ok. Not sure that mono can properly align this type to 64 bytes.

@tannergooding
Copy link
Member Author

Not sure that mono can properly align this type to 64 bytes.

Mono has a different tracking issue covering its need to properly pad all of the Vector types. Noting that padding and alignment are two different things here. RyuJIT correctly "pads" these types (e.g. if you have int x; Vector512<int> y;, y is at offset 64). It does not guarantee correct alignment for GC tracked data (e.g. Vector512<int>[] will not be 64-byte aligned in memory) but will attempt to correctly align stack locals (e.g. Vector512<int> local; should generally be aligned within a given method's stack).

Copy link
Contributor

@dakersnar dakersnar left a comment

Choose a reason for hiding this comment

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

Looks good to me. I only skimmed the tests, but they look comprehensive, and I'm assuming you have full coverage.

Added a few clarifying comments but overall approving this.

@stephentoub
Copy link
Member

@tannergooding, is this mergable now? Thanks.

@tannergooding tannergooding added the blocked Issue/PR is blocked on something - see comments label Nov 3, 2022
@tannergooding
Copy link
Member Author

@stephentoub, was asked to wait until at least end of the week by @davidwrighton so #74886 could go in first. Marked this "blocked" for now.

@stephentoub
Copy link
Member

Got it, thanks.

Copy link
Contributor

@dakersnar dakersnar left a comment

Choose a reason for hiding this comment

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

Reviewing the newest commits. Are all of the newly exposed methods unit tested already?

@tannergooding
Copy link
Member Author

@fanyang-mono, I've logged #80467 and disabled the tests under <ItemGroup Condition="'$(RuntimeFlavor)' == 'mono' and '$(RuntimeVariant)' == 'llvmfullaot' ">

@tannergooding
Copy link
Member Author

Arm64 CI timeouts appear to be a broader issue atm, but I'd still like to get llvmfullaot passing at least once since it hadn't been passing previously.

@tannergooding tannergooding dismissed davidwrighton’s stale review January 12, 2023 16:33

Feedback has been addressed and new review was given by other R2R/Crossgen owners

@tannergooding tannergooding merged commit 00596e5 into dotnet:main Jan 12, 2023
@tannergooding tannergooding deleted the vector512 branch January 12, 2023 16:33
Comment on lines -312 to -314
} else if ((fsig->params [0]->type == MONO_TYPE_GENERICINST) && (fsig->params [1]->type == MONO_TYPE_GENERICINST)) {
instc0 = OP_FDIV;
break;
Copy link
Member

Choose a reason for hiding this comment

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

I am curious, why was this and similar branch in the multiply case removed? It is disabling SIMD code generation for Vector128 div/mul ops for wasm and probably more on the other platforms too. It would help me to find out how to fix it.

@lewing is meanwhile trying to re-enable it in this draft #81201 to see if CI would complain.

Copy link
Member

Choose a reason for hiding this comment

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

This looks like a merge mistake to me. The correct version should be #78784.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like it was a bad merge into this PR. The correct code is in 8158cb8be51 (from #78784) but then when the PR was merged an incomplete version from this PR was picked instead during the merge (67abfd0)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for that. I don't remember a merge conflict coming up, so I'd guess this was automatically done on git's end.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, thanks for looking into that and confirming it.

@@ -339,9 +336,6 @@ emit_simd_ins_for_binary_op (MonoCompile *cfg, MonoClass *klass, MonoMethodSigna
ins = emit_simd_ins (cfg, klass, OP_XBINOP_BYSCALAR, ins->dreg, args [1]->dreg);
ins->inst_c0 = OP_FMUL;
return ins;
} else if ((fsig->params [0]->type == MONO_TYPE_GENERICINST) && (fsig->params [1]->type == MONO_TYPE_GENERICINST)) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 25, 2023
@jeffhandley jeffhandley added the blog-candidate Completed PRs that are candidate topics for blog post coverage label Mar 14, 2023
@ericstj ericstj added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Oct 18, 2023
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 18, 2023
@ericstj ericstj removed breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Oct 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.Intrinsics avx512 Related to the AVX-512 architecture blog-candidate Completed PRs that are candidate topics for blog post coverage new-api-needs-documentation
Projects
None yet