From 9ec4e9095a2562f20476844824f596647d92f34c Mon Sep 17 00:00:00 2001 From: Jakob Kummerow Date: Thu, 26 Jan 2023 15:19:03 +0100 Subject: [PATCH] [turbofan] Fix 32-to-64 bit spill slot moves Other optimizations can create a situation where it is valid to treat a stack slot as either 32-bit (which is what its value was created as) or 64-bit value (to which it was implicitly zero-extended). So when moving such a value to a register, we cannot use a 32-bit move instruction just because the source was annotated as such; we must also take the target slot's representation into account. Fixed: chromium:1407594 Bug: chromium:1356461 Change-Id: I00d850c11a020b055e90f6107b604cdd267d9b6c Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4197349 Auto-Submit: Jakob Kummerow Reviewed-by: Maya Lekova Commit-Queue: Maya Lekova Commit-Queue: Jakob Kummerow Cr-Commit-Position: refs/heads/main@{#85501} --- .../backend/x64/code-generator-x64.cc | 24 +++++-- .../regress/wasm/regress-crbug-1407594.js | 64 +++++++++++++++++++ 2 files changed, 82 insertions(+), 6 deletions(-) create mode 100644 test/mjsunit/regress/wasm/regress-crbug-1407594.js diff --git a/src/compiler/backend/x64/code-generator-x64.cc b/src/compiler/backend/x64/code-generator-x64.cc index 6390e33583..00c976d986 100644 --- a/src/compiler/backend/x64/code-generator-x64.cc +++ b/src/compiler/backend/x64/code-generator-x64.cc @@ -5220,6 +5220,18 @@ void CodeGenerator::SetPendingMove(MoveOperands* move) { } } +namespace { + +bool Is32BitOperand(InstructionOperand* operand) { + DCHECK(operand->IsStackSlot() || operand->IsRegister()); + MachineRepresentation mr = LocationOperand::cast(operand)->representation(); + return mr == MachineRepresentation::kWord32 || + mr == MachineRepresentation::kCompressed || + mr == MachineRepresentation::kCompressedPointer; +} + +} // namespace + void CodeGenerator::AssembleMove(InstructionOperand* source, InstructionOperand* destination) { X64OperandConverter g(this, nullptr); @@ -5343,18 +5355,18 @@ void CodeGenerator::AssembleMove(InstructionOperand* source, case MoveType::kStackToRegister: { Operand src = g.ToOperand(source); if (source->IsStackSlot()) { - MachineRepresentation mr = - LocationOperand::cast(source)->representation(); - const bool is_32_bit = mr == MachineRepresentation::kWord32 || - mr == MachineRepresentation::kCompressed || - mr == MachineRepresentation::kCompressedPointer; // TODO(13581): Fix this for other code kinds (see // https://crbug.com/1356461). - if (code_kind() == CodeKind::WASM_FUNCTION && is_32_bit) { + if (code_kind() == CodeKind::WASM_FUNCTION && Is32BitOperand(source) && + Is32BitOperand(destination)) { // When we need only 32 bits, move only 32 bits. Benefits: // - Save a byte here and there (depending on the destination // register; "movl eax, ..." is smaller than "movq rax, ..."). // - Safeguard against accidental decompression of compressed slots. + // We must check both {source} and {destination} to be 32-bit values, + // because treating 32-bit sources as 64-bit values can be perfectly + // fine as a result of virtual register renaming (to avoid redundant + // explicit zero-extensions that also happen implicitly). __ movl(g.ToRegister(destination), src); } else { __ movq(g.ToRegister(destination), src); diff --git a/test/mjsunit/regress/wasm/regress-crbug-1407594.js b/test/mjsunit/regress/wasm/regress-crbug-1407594.js new file mode 100644 index 0000000000..943ba85be4 --- /dev/null +++ b/test/mjsunit/regress/wasm/regress-crbug-1407594.js @@ -0,0 +1,64 @@ +// Copyright 2023 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +d8.file.execute('test/mjsunit/wasm/wasm-module-builder.js'); + +let builder = new WasmModuleBuilder(); +builder.addMemory(1, 1, false); +builder.addDataSegment(0, [0x78, 0x56, 0x34, 0x12]); + +let spiller = builder.addFunction('spiller', kSig_v_v).addBody([]); + +builder.addFunction('main', kSig_l_v) + .exportFunc() + .addLocals(kWasmI64, 1) + .addBody([ + // Initialize {var0} to 0x12345678 via a zero-extended 32-bit load. + kExprI32Const, 0, + kExprI64LoadMem32U, 2, 0, + kExprLocalSet, 0, + kExprCallFunction, spiller.index, + // The observable effect of this loop is that {var0} is left-shifted + // until it ends in 0x..000000. The details are specifically crafted + // to recreate a particular pattern of spill slot moves. + kExprLoop, kWasmVoid, + kExprI32Const, 0, + kExprI32LoadMem, 2, 0, + kExprI32Eqz, + // This block is never taken; it only serves to influence register + // allocator choices. + kExprIf, kWasmVoid, + kExprLocalGet, 0, + kExprI64Const, 1, + kExprI64And, + kExprLocalSet, 0, + kExprEnd, // if + kExprLocalGet, 0, + kExprI64Const, 1, + kExprI64And, + kExprI64Eqz, + // This block is always taken; it is conditional in order to influence + // register allocator choices. + kExprIf, kWasmVoid, + kExprLocalGet, 0, + kExprI64Const, 8, + kExprI64Shl, + kExprLocalSet, 0, + kExprEnd, // if + kExprBlock, kWasmVoid, + kExprLocalGet, 0, + ...wasmI64Const(0xFFFFFF), + kExprI64And, + kExprI64Eqz, + kExprI32Eqz, + kExprCallFunction, spiller.index, + kExprBrIf, 1, + kExprEnd, // block + kExprCallFunction, spiller.index, + kExprEnd, // loop + kExprLocalGet, 0, + ]); + +let instance = builder.instantiate(); +assertEquals("12345678000000", instance.exports.main().toString(16));