Skip to content

Commit 41472c4

Browse files
EgorBoCopilot
andauthored
Fix bug in LowerCallMemmove (#123907)
Might fix (but not sure) #123748 `LowerCallMemmove` was initially written to inline `SpanHelpers.Memmove` into something that guarantees proper non-aliased move (load all data into separate registers first, then save them all at once) as opposite to what normal `GT_STORE_BLK` does - it's paired with `genCodeForMemmove`. That's why we sort of try to Lower BLK/STORE_BLK by hands there. Later, `CORINFO_HELP_MEMCPY` was added as well (typical scenario: `cpblk` IL code (`Unsafe.CopyBlocksUnaligned`) with the length that becomes a constant in a late phase) and since it doesn't require the memmove semantics, we used `GenTreeBlk::BlkOpKindUnroll`, but in that case we either have to replace the call into a pre-Lower shape and call Lower on srcBlk+storeBlk since we're no longer paired with `genCodeForMemmove`, or just use the same memmove mode. `memmove` mode is more expensive for LSRA and doesn't support addressing modes, but it seems that `CORINFO_HELP_MEMCPY` is rarely used. Example of a changed codegen: ```cs static void Test(ref byte a, ref byte b, uint len) { if (len == 200) { Unsafe.CopyBlockUnaligned(ref a, ref b, len); } } ``` Was: ```asm vmovdqu32 zmm0, zmmword ptr [rdx] vmovdqu32 zmmword ptr [rcx], zmm0 vmovdqu32 zmm0, zmmword ptr [rdx+0x40] vmovdqu32 zmmword ptr [rcx+0x40], zmm0 vmovdqu32 zmm0, zmmword ptr [rdx+0x80] vmovdqu32 zmmword ptr [rcx+0x80], zmm0 mov rax, qword ptr [rdx+0xC0] mov qword ptr [rcx+0xC0], rax ``` Now: ```asm vmovdqu32 zmm0, zmmword ptr [rdx] vmovdqu32 zmm1, zmmword ptr [rdx+0x40] vmovdqu32 zmm2, zmmword ptr [rdx+0x80] vmovdqu xmm3, xmmword ptr [rdx+0xB8] vmovdqu32 zmmword ptr [rcx], zmm0 vmovdqu32 zmmword ptr [rcx+0x40], zmm1 vmovdqu32 zmmword ptr [rcx+0x80], zmm2 vmovdqu xmmword ptr [rcx+0xB8], xmm3 ``` No [diffs](https://dev.azure.com/dnceng-public/public/_build/results?buildId=1287687&view=ms.vss-build-web.run-extensions-tab) --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent 23c8cd9 commit 41472c4

File tree

1 file changed

+6
-6
lines changed

1 file changed

+6
-6
lines changed

src/coreclr/jit/lower.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2468,6 +2468,8 @@ bool Lowering::LowerCallMemmove(GenTreeCall* call, GenTree** next)
24682468

24692469
GenTree* dstAddr = call->gtArgs.GetUserArgByIndex(0)->GetNode();
24702470
GenTree* srcAddr = call->gtArgs.GetUserArgByIndex(1)->GetNode();
2471+
assert(!dstAddr->isContained());
2472+
assert(!srcAddr->isContained());
24712473

24722474
// TODO-CQ: Try to create an addressing mode
24732475
GenTreeIndir* srcBlk = m_compiler->gtNewIndir(TYP_STRUCT, srcAddr);
@@ -2477,11 +2479,8 @@ bool Lowering::LowerCallMemmove(GenTreeCall* call, GenTree** next)
24772479
GenTreeBlk(GT_STORE_BLK, TYP_STRUCT, dstAddr, srcBlk, m_compiler->typGetBlkLayout((unsigned)cnsSize));
24782480
storeBlk->gtFlags |= (GTF_IND_UNALIGNED | GTF_ASG | GTF_EXCEPT | GTF_GLOB_REF);
24792481

2480-
// TODO-CQ: Use GenTreeBlk::BlkOpKindUnroll here if srcAddr and dstAddr don't overlap, thus, we can
2481-
// unroll this memmove as memcpy - it doesn't require lots of temp registers
2482-
storeBlk->gtBlkOpKind = call->IsHelperCall(m_compiler, CORINFO_HELP_MEMCPY)
2483-
? GenTreeBlk::BlkOpKindUnroll
2484-
: GenTreeBlk::BlkOpKindUnrollMemmove;
2482+
// For simplicity, we use BlkOpKindUnrollMemmove even for CORINFO_HELP_MEMCPY.
2483+
storeBlk->gtBlkOpKind = GenTreeBlk::BlkOpKindUnrollMemmove;
24852484

24862485
BlockRange().InsertBefore(call, srcBlk);
24872486
BlockRange().InsertBefore(call, storeBlk);
@@ -2499,7 +2498,8 @@ bool Lowering::LowerCallMemmove(GenTreeCall* call, GenTree** next)
24992498

25002499
JITDUMP("\nNew tree:\n")
25012500
DISPTREE(storeBlk);
2502-
// TODO: This skips lowering srcBlk and storeBlk.
2501+
// We've just lowered srcBlk and storeBlk here and it's now what genCodeForMemmove expects.
2502+
// So the next node to lower is whatever we have after the storeBlk.
25032503
*next = storeBlk->gtNext;
25042504
return true;
25052505
}

0 commit comments

Comments
 (0)