Use SIMD operations in InitBlkUnroll/CopyBlkUnroll and increase unroll limit up to 128 bytes #61030
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
echesakov
added
arch-arm64
and removed
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue Detailsbenchmarks.run.windows.arm64.checked.mch:Detail diffscoreclr_tests.pmi.windows.arm64.checked.mch:Detail diffslibraries.crossgen2.windows.arm64.checked.mch:Detail diffslibraries.pmi.windows.arm64.checked.mch:Detail diffslibraries_tests.pmi.windows.arm64.checked.mch:Detail diffs
|
echesakov
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue Detailsbenchmarks.run.windows.arm64.checked.mch:Detail diffscoreclr_tests.pmi.windows.arm64.checked.mch:Detail diffslibraries.crossgen2.windows.arm64.checked.mch:Detail diffslibraries.pmi.windows.arm64.checked.mch:Detail diffslibraries_tests.pmi.windows.arm64.checked.mch:Detail diffs
|
|
I am also adding micro benchmarks to test performance impact of changes surrounding FWIW, the current change is mostly CQ improvements with little effect on the performance. As you can notice, I am not changing The following are the results of running the new micro benchmarks with the proposed increased
|
echesakov
force-pushed
the
Arm64-Unroll-Init-Block
branch
from
94eaaaa
to
3c84b5a
Compare
echesakov
force-pushed
the
Arm64-Unroll-Init-Block
branch
from
929dc5d
to
5bc4a6e
Compare
This was referenced Nov 10, 2021
echesakov
force-pushed
the
Arm64-Unroll-Init-Block
branch
from
5bc4a6e
to
653c8c0
Compare
echesakov
changed the title
echesakov
force-pushed
the
Arm64-Unroll-Init-Block
branch
from
653c8c0
to
7063255
Compare
echesakov
force-pushed
the
Arm64-Unroll-Init-Block
branch
from
7063255
to
c3566b1
Compare
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…lkUnroll() in src/coreclr/jit/codegenarmarch.cpp
…r/jit/codegenarmarch.cpp
…pyBlockUnrollHelper in src/coreclr/jit/codegenarmarch.cpp
…eg for InitBlock in src/coreclr/jit/lsraarmarch.cpp
…eg for CopyBlock in src/coreclr/jit/lsraarmarch.cpp
…adOrStorePairOffset() helper in src/coreclr/jit/emitarm64.h src/coreclr/jit/emitarm64.cpp
…lr/jit/codegenarmarch.cpp
|
This pull request has been automatically marked |
…r are contained and can potentially occupy up to 2 int registers
ghost
removed
no-recent-activity
needs-author-action
|
@BruceForstall Please take another look. I think I addressed your feedback. |
|
/azp run runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
sandreenko
reviewed
Dec 29, 2021
| static unsigned GetRegSizeAtLeastBytes(unsigned byteCount) | ||
| { | ||
| assert(byteCount != 0); | ||
| assert(byteCount < 16); |
There was a problem hiding this comment.
would not be assert(byteCount <= 16); less confusing?
|
|
||
| void StorePairRegs(int offset, unsigned regSizeBytes) | ||
| { | ||
| canEncodeAllStores = |
There was a problem hiding this comment.
nit canEncodeAllStores &| ? It is safe for bool-s in C++.
| bool canEncodeAllStores; | ||
| }; | ||
|
|
||
| class ProducingStreamBaseInstrs |
There was a problem hiding this comment.
does base mean no-simd?
|
|
||
| private: | ||
| template <class InstructionStream> | ||
| void UnrollInitBlock(InstructionStream& instrStream, int initialRegSizeBytes) const |
There was a problem hiding this comment.
code convention forbids non-const ref args https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/clr-jit-coding-conventions.md#1535-reference-arguments
| intReg1 = node->ExtractTempReg(RBM_ALLINT); | ||
| intReg2 = node->GetSingleTempReg(RBM_ALLINT); | ||
| break; | ||
| default: |
There was a problem hiding this comment.
are this default and the next one for simdRegCount unreached()?
| { | ||
| public: | ||
| ProducingStreamBaseInstrs(regNumber intReg1, regNumber intReg2, regNumber addrReg, emitter* emitter) | ||
| : intReg1(intReg1), intReg2(intReg2), addrReg(addrReg), emitter(emitter) |
There was a problem hiding this comment.
should all reg be not REG_NA or does it allow some to be REG_NA?
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently,
CodeGen::genCodeForInitBlkUnroll()andCodeGen::genCodeForCpBlkUnroll()do not use SIMD instructions when generating code forInitBlock/CopyBlockoperations on Arm and Arm64.This PR adds support for that on Arm64.
In order to accurately estimate whether the use of SIMD instructions is needed and/or whether the base address of a block needs to computed and stored to a integer register the algorithm does multiple passes - (one that verifies whether all offsets can be encoded and another that counts how many instructions will be emitted).
The following are justifications why such approach was chosen:
In fact, that issue was already pointed out by a TODO comment (that is removed by the PR)
In other words it transforms a problem of code-generating of
InitBlock(largeOffset, byteCount)toInitBlock(0, byteCount). In order to do figure out that such transformation is required, the algorithm needs a pass to verify whether all the offsets are encodable.For example, the following snippet shows such case
InitBlockAllZeros(startOffset=8, byteCount=16)) since the JIT would need to initialize a SIMD register while it could just use astr xzr, xzr, [dstReg, #dstOffset]instead (assumingdstOffsetis encodable). In order to figure out such cases I've chose to use the same mechanism of multiple passes instead of trying to "encode" all corner cases.As an example, the following demonstrates a case where it is beneficial to "spill" that the base of address of destination in
CopyBlockand use SIMD instructions than use integer instructions without spilling.In addition to that, the instruction selection strategy that the current implementation uses can be characterized as greedy "best fit" and it doesn't always produce the optimal result (for example,
InitBlocksequence for 7 bytes isstr rInitValue, [rAddr]; strh rInitValue, [rAddr+4]; strb rInitValue, [rAddr+6]). I changed the strategy slightly and added support to allow overlapping stores (so that sequence would becomestr rInitValue, [rAddr]; stur rInitValue, [rAddr+3].The following is an example where using overlapping loads/stores result in shorter instruction sequence
ldp x1, x3, [x0] stp x1, x3, [x2,#16] - ldr w1, [x0,#16] - str w1, [x2,#32] - ldrh w1, [x0,#20] - strh w1, [x2,#36] + ldr x1, [x0,#14] + str x1, [x2,#30]