Adding support for vector constants via GenTreeVecCon #68874
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsThis is a draft that adds direct support for 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
|
|
Will re-open after I get tests passing. |
tannergooding
force-pushed
the
vector-cns
branch
5 times, most recently
from
4921110
to
360ff7e
Compare
|
CC. @dotnet/jit-contrib This is needed to correctly handle the 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 |
src/coreclr/jit/valuenum.h
Outdated
| else | ||
| { | ||
| assert(false); | ||
| return {}; |
There was a problem hiding this comment.
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.
Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
There was a problem hiding this comment.
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!
|
CC. @dotnet/jit-contrib. This should be ready for review. |
There was a problem hiding this comment.
@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.
ghost
locked as resolved and limited conversation to collaborators
|
@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?
System.Numerics.Tests.Perf_Matrix4x4.CreateShadowBenchmark
|
|
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. |
This adds direct support for
vector constantnodes viaGenTreeVecCon. It involved quite a bit of cleanup to normalize the places that were touchingGT_SIMD, those that were touchingGT_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.