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

[vm] register pressure MemoryCopyInstr on arm32 with element size 16 #51229

Open
dcharkes opened this issue Feb 2, 2023 · 1 comment
Open
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends.

Comments

@dcharkes
Copy link
Contributor

dcharkes commented Feb 2, 2023

../../runtime/vm/compiler/backend/linearscan.cc: 2492: error: expected: unallocated->Start() < ToInstructionStart(register_use_pos)

While trying to add a unit test which uses the mem-copy instruction with element-size 16, the register allocator for arm32 ran out of registers. (It's trying to spill from the same location as the first use, a zero-length spill.)

We only have 16 registers in total, 8 of them are pinned in the Dart calling convention.

enum Register {
R0 = 0,
R1 = 1,
R2 = 2,
R3 = 3,
R4 = 4,
R5 = 5, // PP
R6 = 6, // CODE_REG
R7 = 7, // FP on iOS, DISPATCH_TABLE_REG on non-iOS (AOT only)
R8 = 8,
R9 = 9,
R10 = 10, // THR
R11 = 11, // FP on non-iOS, DISPATCH_TABLE_REG on iOS (AOT only)
R12 = 12, // IP aka TMP
R13 = 13, // SP
R14 = 14, // LR
R15 = 15, // PC

With element 16, this instruction requires 9 registers (4 temps, and 5 parameters).

LocationSummary* MemoryCopyInstr::MakeLocationSummary(Zone* zone,
bool opt) const {
const intptr_t kNumInputs = 5;
const intptr_t kNumTemps = element_size_ == 16 ? 4
: element_size_ == 8 ? 2
: 1;
LocationSummary* locs = new (zone)
LocationSummary(zone, kNumInputs, kNumTemps, LocationSummary::kNoCall);
locs->set_in(kSrcPos, Location::WritableRegister());
locs->set_in(kDestPos, Location::WritableRegister());
locs->set_in(kSrcStartPos, Location::RequiresRegister());
locs->set_in(kDestStartPos, Location::RequiresRegister());
locs->set_in(kLengthPos, Location::WritableRegister());
for (intptr_t i = 0; i < kNumTemps; i++) {
locs->set_temp(i, Location::RequiresRegister());
}
return locs;
}

Since we're currently not exercising anything else than element-size 1, we'll not hit it in Dart code right now, but we should fix this.

Possible fix:

  • Do the copy with 8 bytes and shift the length one to the left.
@dcharkes dcharkes added the area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. label Feb 2, 2023
@rmacnak-google
Copy link
Contributor

Could use the SIMD registers as the temps for the larger sizes. I see that gcc compiles a 16-byte copy with SIMD loads and stores:

void copy(uint64_t* src, uint64_t* dst) {
    uint64_t a = src[0];
    uint64_t b = src[1];
    dst[0] = a;
    dst[1] = b;
}
vld1.64 {d16-d17}, [r0:64]
vst1.64 {d16-d17}, [r1:64]
bx      lr

copybara-service bot pushed a commit that referenced this issue Sep 4, 2023
When setRange is called on a TypedData receiver and the source is also
a TypedData object with the same element size and clamping is not
required, the VM implementation now calls _boundsCheckAndMemcpyN for
element size N. The generated IL for these methods performs the copy
using the MemoryCopy instruction (mostly, see the note below).

Since the two TypedData objects might have the same underlying
buffer, the CL adds a can_overlap flag to the MemoryCopy instruction
which checks for overlapping regions. If can_overlap is set, then
the copy is performed backwards instead of forwards when needed
to ensure that elements of the source region are read before
they are overwritten.

The existing uses of the MemoryCopy instruction are adjusted as
follows:
* The IL generated for copyRangeFromUint8ListToOneByteString
  passes false for can_overlap, as all uses currently ensure that
  the OneByteString is non-external and thus cannot overlap.
* The IL generated for _memCopy, used by the FFI library, passes
  true for can_overlap, as there is no guarantee that the regions
  pointed at by the Pointer objects do not overlap.

The MemoryCopy instruction has also been adjusted so that all numeric
inputs (the two start offsets and the length) are either boxed or
unboxed instead of just the length. This exposed an issue
in the inliner, where unboxed constants in the callee graph were
replaced with boxed constants when inlining into the caller graph,
since withList calls setRange with constant starting offsets of 0.
Now the representation of constants in the callee graph are preserved
when inlining the callee graph into the caller graph.

Fixes #51237 by using TMP
and TMP2 for the LDP/STP calls in the 16-byte element size case, so no
temporaries need to be allocated for the instruction.

On ARM when not unrolling the memory copy loop, uses TMP and a single
additional temporary for LDM/STM calls in the 8-byte and 16-byte
element cases, with the latter just using two LDM/STM calls within
the loop, a different approach than the one described in
#51229 .

Note: Once the number of elements being copied reaches a certain
threshold (1048576 on X86, 256 otherwise), _boundsCheckAndMemcpyN
instead calls _nativeSetRange, which is a native call that uses memmove
from the standard C library for non-clamped inputs. It does this
because the code currently emitted for MemoryCopy performs poorly
compared to the more optimized memmove implementation when copying
larger regions of memory.

Notable benchmark changes for dart-aot:
* X64
  * TypedDataDuplicate.*.fromList improvement from ~13%-~250%
  * Uf8Encode.*.10 improvement from ~50%-~75%
  * MapCopy.Map.*.of.Map.* improvement from ~13%-~65%
  * MemoryCopy.*.setRange.* improvement from ~13%-~500%
* ARM7
  * Uf8Encode.*.10 improvement from ~35%-~70%
  * MapCopy.Map.*.of.Map.* improvement from ~6%-~75%
  * MemoryCopy.*.setRange.{8,64} improvement from ~22%-~500%
    * Improvement of ~100%-~200% for MemoryCopy.512.setRange.*.Double
    * Regression of ~40% for MemoryCopy.512.setRange.*.Uint8
    * Regression of ~85% for MemoryCopy.4096.setRange.*.Uint8
* ARM8
  * Uf8Encode.*.10 improvement from ~35%-~70%
  * MapCopy.Map.*.of.Map.* improvement from ~7%-~75%
  * MemoryCopy.*.setRange.{8,64} improvement from ~22%-~500%
    * Improvement of ~75%-~160% for MemoryCopy.512.setRange.*.Double
    * Regression of ~40% for MemoryCopy.512.setRange.*.Uint8
    * Regression of ~85% for MemoryCopy.4096.setRange.*.Uint8

TEST=vm/cc/IRTest_Memory, co19{,_2}/LibTest/typed_data,
     lib{,_2}/typed_data, corelib{,_2}/list_test

Issue: #42072
Issue: b/294114694
Issue: b/259315681

Change-Id: Ic75521c5fe10b952b5b9ce5f2020c7e3f03672a9
Cq-Include-Trybots: luci.dart.try:vm-aot-linux-debug-simarm_x64-try,vm-aot-linux-debug-simriscv64-try,vm-aot-linux-debug-x64-try,vm-aot-linux-debug-x64c-try,vm-kernel-linux-debug-x64-try,vm-kernel-precomp-linux-debug-x64-try,vm-linux-debug-ia32-try,vm-linux-debug-simriscv64-try,vm-linux-debug-x64-try,vm-linux-debug-x64c-try,vm-mac-debug-arm64-try,vm-mac-debug-x64-try,vm-aot-linux-release-simarm64-try,vm-aot-linux-release-simarm_x64-try,vm-aot-linux-release-x64-try,vm-aot-mac-release-arm64-try,vm-aot-mac-release-x64-try,vm-ffi-qemu-linux-release-riscv64-try,vm-ffi-qemu-linux-release-arm-try,vm-aot-msan-linux-release-x64-try,vm-msan-linux-release-x64-try,vm-aot-tsan-linux-release-x64-try,vm-tsan-linux-release-x64-try,vm-linux-release-ia32-try,vm-linux-release-simarm-try,vm-linux-release-simarm64-try,vm-linux-release-x64-try,vm-mac-release-arm64-try,vm-mac-release-x64-try,vm-kernel-precomp-linux-release-x64-try,vm-aot-android-release-arm64c-try,vm-ffi-android-debug-arm64c-try
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/319521
Reviewed-by: Daco Harkes <dacoharkes@google.com>
Reviewed-by: Alexander Markov <alexmarkov@google.com>
Commit-Queue: Tess Strickland <sstrickl@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends.
Projects
None yet
Development

No branches or pull requests

2 participants