Skip to content

Adding support for vector constants via GenTreeVecCon - #68874

Merged
tannergooding merged 17 commits into
dotnet:mainfrom
tannergooding:vector-cns
May 31, 2022
Merged

Adding support for vector constants via GenTreeVecCon#68874
tannergooding merged 17 commits into
dotnet:mainfrom
tannergooding:vector-cns

Conversation

@tannergooding

@tannergoodingtannergooding commented May 4, 2022

Copy link
Copy Markdown
Member

This adds direct support for vector constant nodes via GenTreeVecCon. It involved quite a bit of cleanup to normalize the places that were touching GT_SIMD, those that were touching GT_HWINTRINSIC, and those that touched both so it grew up a bit more than I initially desired.

The result, overall, however is that vector constants are now centrally handled with less overall allocations and nodes required to represent them. They are also now base type independent which allows more CSE opportunities and which means that you can easily see the bits however the user needs to interpret them. This in turn will simplify logic for the xplat shuffle APIs where otherwise simple operations, such as .AsInt32() would break the handling and force it down the fallback path.

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 4, 2022
@ghost

ghost commented May 4, 2022

Copy link
Copy Markdown

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

Issue Details

This is a draft that adds direct support for vector constant nodes via GenTreeVecCon. It involved quite a bit of cleanup to normalize the places that were touching GT_SIMD, those that were touching GT_HWINTRINSIC, and those that touched both so it grew up a bit more than I initially desired.

The result, overall, however is that vector constants are now centrally handled with less overall allocations and nodes required to represent them. They are also now base type independent which allows more CSE opportunities and which means that you can easily see the bits however the user needs to interpret them. This in turn will simplify logic for the xplat shuffle APIs where otherwise simple operations, such as .AsInt32() would break the handling and force it down the fallback path.

Author:tannergooding
Assignees:tannergooding
Labels:

area-CodeGen-coreclr

Milestone:-

@tannergooding

Copy link
Copy Markdown
MemberAuthor

Will re-open after I get tests passing.

@tannergooding
tannergoodingforce-pushed the vector-cns branch 5 times, most recently from 4921110 to 360ff7eCompareMay 9, 2022 14:40
@tannergooding
tannergooding marked this pull request as ready for review May 10, 2022 20:19
@tannergooding

Copy link
Copy Markdown
MemberAuthor

CC. @dotnet/jit-contrib

This is needed to correctly handle the Shuffle APIs and some of the more complex patterns around vector constant nodes that pop up (part of the remaining xplat hwintrinsic APIs: #63331).

It provides some throughput gains on all platforms and spot-checking the regressions they are mostly from new CSE opportunities and other optimizations that can now kick in due to knowing these are constant. There are a couple regressions where we emit additional xorps instructions since its considered cheap enough CSE doesn't always kick in (this issue is more broadly tracked by #6264).

Comment threadsrc/coreclr/jit/valuenum.cpp
Comment threadsrc/coreclr/jit/valuenum.h Outdated
else
{
assert(false);
return {};

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 define a simd8/16/32_t::Zero-like static constexpr field (or function), semantics of { } can be unclear to people not intimately familiar with the C++ initialization rules.

Comment threadsrc/coreclr/jit/assertionprop.cpp Outdated

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

Some initial feedback; will continue the review tomorrow.

Comment threadsrc/coreclr/jit/rationalize.cpp Outdated
Comment threadsrc/coreclr/jit/valuenum.cpp
Comment threadsrc/coreclr/jit/assertionprop.cpp Outdated
Comment threadsrc/coreclr/jit/emit.cpp
Comment threadsrc/coreclr/jit/importer.cpp Outdated
Comment threadsrc/coreclr/jit/morph.cpp Outdated
Comment threadsrc/coreclr/jit/rationalize.cpp Outdated
Comment threadsrc/coreclr/jit/valuenum.h Outdated
Comment threadsrc/coreclr/jit/gentree.h Outdated
Comment threadsrc/coreclr/jit/gentree.h Outdated
Comment threadsrc/coreclr/jit/lsraarm64.cpp
Comment threadsrc/coreclr/jit/lowerarmarch.cpp Outdated
Comment threadsrc/coreclr/jit/lowerarmarch.cpp
Comment threadsrc/coreclr/jit/lowerloongarch64.cpp
Comment threadsrc/coreclr/jit/hwintrinsicxarch.cpp Outdated
Comment threadsrc/coreclr/jit/assertionprop.cpp Outdated
Comment threadsrc/coreclr/jit/gentree.h Outdated
Comment threadsrc/coreclr/jit/lowerxarch.cpp Outdated
Comment threadsrc/coreclr/jit/lowerxarch.cpp Outdated
Comment threadsrc/coreclr/jit/lowerxarch.cpp Outdated
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>

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

The frontend/IR changes look good. I did not drill too much into the backend parts, but they look mostly mechanical (and correct).

Overall, I think this change is a good step in the right direction w.r.t. SIMDs in IR and removes a good amount of suboptimal representation that we had for constant vectors. Thank you for making it happen!

@tannergooding

Copy link
Copy Markdown
MemberAuthor

CC. @dotnet/jit-contrib. This should be ready for review.

Comment threadsrc/coreclr/jit/assertionprop.cpp

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

@SingleAccretion did an incredible review of this.

The changes look great. Only a minor comment regarding the CORINFO_TYPE_FLOAT.

For ARM64 diffs, the regressions seem to just appear as regressions due to new CSE opportunities - so that all looks good to me.

@mrsharm

Copy link
Copy Markdown
Member

@tannergooding - from our analysis while creating the perf report for August, we found the following regression that seemed to line up with this PR specifically related to the Ubuntu 18.04 x64 configuration. Would you consider these regressions as "by design" as there are closed auto-filed regressions above?

image

Details

System.Numerics.Tests.Perf_Matrix4x4.CreateShadowBenchmark

ResultRatioAlloc DeltaOperating SystemBitProcessor Name
Same1.00+0Windows 11Arm64Microsoft SQ1 3.0 GHz
Same1.01+0Windows 11Arm64Microsoft SQ1 3.0 GHz
Slower0.29+0macOS Monterey 12.3Arm64Apple M1 Max
Same1.01+0Windows 10X64Intel Xeon CPU E5-1650 v4 3.60GHz
Same1.03+0Windows 10X64Intel Core i7-6700 CPU 3.40GHz (Skylake)
Same1.04+0Windows 10X64Intel Core i7-6700 CPU 3.40GHz (Skylake)
Same0.99+0Windows 10X64Intel Core i7-8650U CPU 1.90GHz (Kaby Lake R)
Same1.02+0Windows 10X64Intel Core i9-10900K CPU 3.70GHz
Same1.06+0Windows 11X64AMD Ryzen Threadripper PRO 3945WX 12-Cores
Same1.03+0Windows 11X64AMD Ryzen 9 3950X
Same0.98+0Windows 11X64AMD Ryzen 9 5900X
Same0.95+0Windows 11X64AMD Ryzen 9 5950X
Same0.94+0Windows 11X64Intel Core i7-8700 CPU 3.20GHz (Coffee Lake)
Same0.95+0Windows 11X64Intel Core i9-10900K CPU 3.70GHz
Same0.92+0Windows 11X6411th Gen Intel Core i9-11900H 2.50GHz
Slower0.68+0ubuntu 18.04X64Intel Xeon CPU E5-1650 v4 3.60GHz
Slower0.86+0ubuntu 18.04X64Intel Core i7-2720QM CPU 2.20GHz (Sandy Bridge)
Slower0.62+0ubuntu 18.04X64Intel Core i7-8700 CPU 3.20GHz (Coffee Lake)
Slower0.68+0ubuntu 20.04X64AMD Ryzen 9 5900X
Slower0.58+0ubuntu 20.04X64Intel Core i9-10900K CPU 3.70GHz
Faster1.54+0Windows 10X86Intel Xeon CPU E5-1650 v4 3.60GHz
Same1.00+0Windows 10X86Intel Core i7-6700 CPU 3.40GHz (Skylake)
Same0.99+0Windows 11X86AMD Ryzen Threadripper PRO 3945WX 12-Cores
Slower0.65+0macOS Big Sur 11.6.8X64Intel Core i5-4278U CPU 2.60GHz (Haswell)
Slower0.64+0macOS Monterey 12.3.1X64Intel Core i7-5557U CPU 3.10GHz (Broadwell)
Slower0.65+0macOS Monterey 12.4X64Intel Core i5-4278U CPU 2.60GHz (Haswell)

@tannergooding

Copy link
Copy Markdown
MemberAuthor

Not by design, would need to see the disassembly to see exactly what's being pessimized here.

This isn't important for .NET 7, however. Matrix4x4.CreateShadow is a case that isn't accelerated today and where the overall codegen is already suboptimal. The "proper" fix would be to rewrite the implementation to properly take advantage of the hardware intrinsics where possible.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@tannergooding@AndyAyersMS@mrsharm@TIHan@SingleAccretion@JulieLeeMSFT