[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}

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);

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));