Skip to content

Commit 9ec4e90

Browse files
jakobkummerowV8 LUCI CQ
jakobkummerow
authored and
V8 LUCI CQ
committed
[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 <[email protected]> Reviewed-by: Maya Lekova <[email protected]> Commit-Queue: Maya Lekova <[email protected]> Commit-Queue: Jakob Kummerow <[email protected]> Cr-Commit-Position: refs/heads/main@{#85501}
1 parent 83e2bce commit 9ec4e90

2 files changed

Lines changed: 82 additions & 6 deletions

File tree

‎src/compiler/backend/x64/code-generator-x64.cc‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5220,6 +5220,18 @@ void CodeGenerator::SetPendingMove(MoveOperands* move) {
52205220
}
52215221
}
52225222

5223+
namespace {
5224+
5225+
bool Is32BitOperand(InstructionOperand* operand) {
5226+
DCHECK(operand->IsStackSlot() || operand->IsRegister());
5227+
MachineRepresentation mr = LocationOperand::cast(operand)->representation();
5228+
return mr == MachineRepresentation::kWord32 ||
5229+
mr == MachineRepresentation::kCompressed ||
5230+
mr == MachineRepresentation::kCompressedPointer;
5231+
}
5232+
5233+
} // namespace
5234+
52235235
void CodeGenerator::AssembleMove(InstructionOperand* source,
52245236
InstructionOperand* destination) {
52255237
X64OperandConverter g(this, nullptr);
@@ -5343,18 +5355,18 @@ void CodeGenerator::AssembleMove(InstructionOperand* source,
53435355
case MoveType::kStackToRegister: {
53445356
Operand src = g.ToOperand(source);
53455357
if (source->IsStackSlot()) {
5346-
MachineRepresentation mr =
5347-
LocationOperand::cast(source)->representation();
5348-
const bool is_32_bit = mr == MachineRepresentation::kWord32 ||
5349-
mr == MachineRepresentation::kCompressed ||
5350-
mr == MachineRepresentation::kCompressedPointer;
53515358
// TODO(13581): Fix this for other code kinds (see
53525359
// https://crbug.com/1356461).
5353-
if (code_kind() == CodeKind::WASM_FUNCTION && is_32_bit) {
5360+
if (code_kind() == CodeKind::WASM_FUNCTION && Is32BitOperand(source) &&
5361+
Is32BitOperand(destination)) {
53545362
// When we need only 32 bits, move only 32 bits. Benefits:
53555363
// - Save a byte here and there (depending on the destination
53565364
// register; "movl eax, ..." is smaller than "movq rax, ...").
53575365
// - Safeguard against accidental decompression of compressed slots.
5366+
// We must check both {source} and {destination} to be 32-bit values,
5367+
// because treating 32-bit sources as 64-bit values can be perfectly
5368+
// fine as a result of virtual register renaming (to avoid redundant
5369+
// explicit zero-extensions that also happen implicitly).
53585370
__ movl(g.ToRegister(destination), src);
53595371
} else {
53605372
__ movq(g.ToRegister(destination), src);

‎test/mjsunit/regress/wasm/regress-crbug-1407594.js‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
// Copyright 2023 the V8 project authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
d8.file.execute('test/mjsunit/wasm/wasm-module-builder.js');
6+
7+
let builder = new WasmModuleBuilder();
8+
builder.addMemory(1, 1, false);
9+
builder.addDataSegment(0, [0x78, 0x56, 0x34, 0x12]);
10+
11+
let spiller = builder.addFunction('spiller', kSig_v_v).addBody([]);
12+
13+
builder.addFunction('main', kSig_l_v)
14+
.exportFunc()
15+
.addLocals(kWasmI64, 1)
16+
.addBody([
17+
// Initialize {var0} to 0x12345678 via a zero-extended 32-bit load.
18+
kExprI32Const, 0,
19+
kExprI64LoadMem32U, 2, 0,
20+
kExprLocalSet, 0,
21+
kExprCallFunction, spiller.index,
22+
// The observable effect of this loop is that {var0} is left-shifted
23+
// until it ends in 0x..000000. The details are specifically crafted
24+
// to recreate a particular pattern of spill slot moves.
25+
kExprLoop, kWasmVoid,
26+
kExprI32Const, 0,
27+
kExprI32LoadMem, 2, 0,
28+
kExprI32Eqz,
29+
// This block is never taken; it only serves to influence register
30+
// allocator choices.
31+
kExprIf, kWasmVoid,
32+
kExprLocalGet, 0,
33+
kExprI64Const, 1,
34+
kExprI64And,
35+
kExprLocalSet, 0,
36+
kExprEnd, // if
37+
kExprLocalGet, 0,
38+
kExprI64Const, 1,
39+
kExprI64And,
40+
kExprI64Eqz,
41+
// This block is always taken; it is conditional in order to influence
42+
// register allocator choices.
43+
kExprIf, kWasmVoid,
44+
kExprLocalGet, 0,
45+
kExprI64Const, 8,
46+
kExprI64Shl,
47+
kExprLocalSet, 0,
48+
kExprEnd, // if
49+
kExprBlock, kWasmVoid,
50+
kExprLocalGet, 0,
51+
...wasmI64Const(0xFFFFFF),
52+
kExprI64And,
53+
kExprI64Eqz,
54+
kExprI32Eqz,
55+
kExprCallFunction, spiller.index,
56+
kExprBrIf, 1,
57+
kExprEnd, // block
58+
kExprCallFunction, spiller.index,
59+
kExprEnd, // loop
60+
kExprLocalGet, 0,
61+
]);
62+
63+
let instance = builder.instantiate();
64+
assertEquals("12345678000000", instance.exports.main().toString(16));

0 commit comments

Comments
 (0)