Skip to content

Commit

Permalink
deps: V8: cherry-pick 9ec4e9095a25
Browse files Browse the repository at this point in the history
Original commit message:

    [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 <jkummerow@chromium.org>
    Reviewed-by: Maya Lekova <mslekova@chromium.org>
    Commit-Queue: Maya Lekova <mslekova@chromium.org>
    Commit-Queue: Jakob Kummerow <jkummerow@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#85501}

Refs: v8/v8@9ec4e90
PR-URL: #47535
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
  • Loading branch information
kleisauke authored and targos committed May 1, 2023
1 parent a978ca6 commit 8283313
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 7 deletions.
2 changes: 1 addition & 1 deletion common.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@

# Reset this number to 0 on major V8 upgrades.
# Increment by one for each non-official patch applied to deps/v8.
'v8_embedder_string': '-node.17',
'v8_embedder_string': '-node.18',

##### V8 defaults for Node.js #####

Expand Down
24 changes: 18 additions & 6 deletions deps/v8/src/compiler/backend/x64/code-generator-x64.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5172,6 +5172,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);
Expand Down Expand Up @@ -5295,18 +5307,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);
Expand Down
64 changes: 64 additions & 0 deletions deps/v8/test/mjsunit/regress/wasm/regress-crbug-1407594.js
Original file line number Diff line number Diff line change
@@ -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));

0 comments on commit 8283313

Please sign in to comment.