[wasm] Fix AtomicStores to not clobber the output register

Currently AtomicStores use AtomicExchange to store to memory, but
AtomicExchange produces an output that is ignored by the AtomicStore
visitor, a side effect of this is that a register already in use gets
overwritten by the output of the exchange.

BUG:v8:7602

Change-Id: I4ec3107a0a27503611e349e6f56ca9492d05d9f8
Reviewed-on: https://chromium-review.googlesource.com/1134576Reviewed-by: Ben Smith <binji@chromium.org>
Reviewed-by: Bill Budge <bbudge@chromium.org>
Commit-Queue: Deepti Gandluri <gdeepti@chromium.org>
Cr-Commit-Position: refs/heads/master@{#54565}

src/compiler/ia32/instruction-selector-ia32.cc

@@ -1285,6 +1285,40 @@ void VisitWordCompare(InstructionSelector* selector, Node* node,
   VisitWordCompare(selector, node, kIA32Cmp, cont);
 }
 
+void VisitAtomicExchange(InstructionSelector* selector, Node* node,
+                         ArchOpcode opcode, MachineRepresentation rep) {
+  IA32OperandGenerator g(selector);
+  Node* base = node->InputAt(0);
+  Node* index = node->InputAt(1);
+  Node* value = node->InputAt(2);
+
+  AddressingMode addressing_mode;
+  InstructionOperand inputs[3];
+  size_t input_count = 0;
+  if (rep == MachineRepresentation::kWord8) {
+    inputs[input_count++] = g.UseFixed(value, edx);
+  } else {
+    inputs[input_count++] = g.UseUniqueRegister(value);
+  }
+  inputs[input_count++] = g.UseUniqueRegister(base);
+  if (g.CanBeImmediate(index)) {
+    inputs[input_count++] = g.UseImmediate(index);
+    addressing_mode = kMode_MRI;
+  } else {
+    inputs[input_count++] = g.UseUniqueRegister(index);
+    addressing_mode = kMode_MR1;
+  }
+  InstructionOperand outputs[1];
+  if (rep == MachineRepresentation::kWord8) {
+    // Using DefineSameAsFirst requires the register to be unallocated.
+    outputs[0] = g.DefineAsFixed(node, edx);
+  } else {
+    outputs[0] = g.DefineSameAsFirst(node);
+  }
+  InstructionCode code = opcode | AddressingModeField::encode(addressing_mode);
+  selector->Emit(code, 1, outputs, input_count, inputs);
+}
+
 }  // namespace
 
 // Shared routine for word comparison with zero.
@@ -1553,10 +1587,6 @@ void InstructionSelector::VisitWord32AtomicLoad(Node* node) {
 
 void InstructionSelector::VisitWord32AtomicStore(Node* node) {
   IA32OperandGenerator g(this);
-  Node* base = node->InputAt(0);
-  Node* index = node->InputAt(1);
-  Node* value = node->InputAt(2);
-
   MachineRepresentation rep = AtomicStoreRepresentationOf(node->op());
   ArchOpcode opcode = kArchNop;
   switch (rep) {
@@ -1573,32 +1603,11 @@ void InstructionSelector::VisitWord32AtomicStore(Node* node) {
       UNREACHABLE();
       break;
   }
-  AddressingMode addressing_mode;
-  InstructionOperand inputs[4];
-  size_t input_count = 0;
-  if (rep == MachineRepresentation::kWord8) {
-    inputs[input_count++] = g.UseByteRegister(value);
-  } else {
-    inputs[input_count++] = g.UseUniqueRegister(value);
-  }
-  inputs[input_count++] = g.UseUniqueRegister(base);
-  if (g.CanBeImmediate(index)) {
-    inputs[input_count++] = g.UseImmediate(index);
-    addressing_mode = kMode_MRI;
-  } else {
-    inputs[input_count++] = g.UseUniqueRegister(index);
-    addressing_mode = kMode_MR1;
-  }
-  InstructionCode code = opcode | AddressingModeField::encode(addressing_mode);
-  Emit(code, 0, nullptr, input_count, inputs);
+  VisitAtomicExchange(this, node, opcode, rep);
 }
 
 void InstructionSelector::VisitWord32AtomicExchange(Node* node) {
   IA32OperandGenerator g(this);
-  Node* base = node->InputAt(0);
-  Node* index = node->InputAt(1);
-  Node* value = node->InputAt(2);
-
   MachineType type = AtomicOpRepresentationOf(node->op());
   ArchOpcode opcode = kArchNop;
   if (type == MachineType::Int8()) {
@@ -1615,31 +1624,7 @@ void InstructionSelector::VisitWord32AtomicExchange(Node* node) {
     UNREACHABLE();
     return;
   }
-  InstructionOperand outputs[1];
-  AddressingMode addressing_mode;
-  InstructionOperand inputs[3];
-  size_t input_count = 0;
-  if (type == MachineType::Int8() || type == MachineType::Uint8()) {
-    inputs[input_count++] = g.UseFixed(value, edx);
-  } else {
-    inputs[input_count++] = g.UseUniqueRegister(value);
-  }
-  inputs[input_count++] = g.UseUniqueRegister(base);
-  if (g.CanBeImmediate(index)) {
-    inputs[input_count++] = g.UseImmediate(index);
-    addressing_mode = kMode_MRI;
-  } else {
-    inputs[input_count++] = g.UseUniqueRegister(index);
-    addressing_mode = kMode_MR1;
-  }
-  if (type == MachineType::Int8() || type == MachineType::Uint8()) {
-    // Using DefineSameAsFirst requires the register to be unallocated.
-    outputs[0] = g.DefineAsFixed(node, edx);
-  } else {
-    outputs[0] = g.DefineSameAsFirst(node);
-  }
-  InstructionCode code = opcode | AddressingModeField::encode(addressing_mode);
-  Emit(code, 1, outputs, input_count, inputs);
+  VisitAtomicExchange(this, node, opcode, type.representation());
 }
 
 void InstructionSelector::VisitWord32AtomicCompareExchange(Node* node) {

src/compiler/x64/instruction-selector-x64.cc

@@ -1892,29 +1892,6 @@ void VisitAtomicExchange(InstructionSelector* selector, Node* node,
   selector->Emit(code, arraysize(outputs), outputs, arraysize(inputs), inputs);
 }
 
-// Shared routine for Word32/Word64 Atomic Store
-void VisitAtomicStore(InstructionSelector* selector, Node* node,
-                      ArchOpcode opcode) {
-  X64OperandGenerator g(selector);
-  Node* base = node->InputAt(0);
-  Node* index = node->InputAt(1);
-  Node* value = node->InputAt(2);
-  AddressingMode addressing_mode;
-  InstructionOperand index_operand;
-  if (g.CanBeImmediate(index)) {
-    index_operand = g.UseImmediate(index);
-    addressing_mode = kMode_MRI;
-  } else {
-    index_operand = g.UseUniqueRegister(index);
-    addressing_mode = kMode_MR1;
-  }
-  InstructionOperand inputs[] = {g.UseUniqueRegister(value),
-                                 g.UseUniqueRegister(base), index_operand};
-  InstructionCode code = opcode | AddressingModeField::encode(addressing_mode);
-  selector->Emit(code, 0, static_cast<InstructionOperand*>(nullptr),
-                 arraysize(inputs), inputs);
-}
-
 }  // namespace
 
 // Shared routine for word comparison against zero.
@@ -2316,7 +2293,7 @@ void InstructionSelector::VisitWord32AtomicStore(Node* node) {
       UNREACHABLE();
       return;
   }
-  VisitAtomicStore(this, node, opcode);
+  VisitAtomicExchange(this, node, opcode);
 }
 
 void InstructionSelector::VisitWord64AtomicStore(Node* node) {
@@ -2339,7 +2316,7 @@ void InstructionSelector::VisitWord64AtomicStore(Node* node) {
       UNREACHABLE();
       return;
   }
-  VisitAtomicStore(this, node, opcode);
+  VisitAtomicExchange(this, node, opcode);
 }
 
 void InstructionSelector::VisitWord32AtomicExchange(Node* node) {

test/cctest/wasm/test-run-wasm-atomics.cc

@@ -303,6 +303,21 @@ WASM_EXEC_TEST(I32AtomicStoreLoad8U) {
   }
 }
 
+WASM_EXEC_TEST(I32AtomicStoreParameter) {
+  EXPERIMENTAL_FLAG_SCOPE(threads);
+  WasmRunner<uint32_t, uint32_t> r(execution_mode);
+  uint32_t* memory =
+      r.builder().AddMemoryElems<uint32_t>(kWasmPageSize / sizeof(uint32_t));
+  r.builder().SetHasSharedMemory();
+
+  BUILD(r,
+        WASM_ATOMICS_STORE_OP(kExprI32AtomicStore, WASM_ZERO, WASM_GET_LOCAL(0),
+                              MachineRepresentation::kWord8),
+        WASM_ATOMICS_BINOP(kExprI32AtomicAdd, WASM_I32V_1(0), WASM_GET_LOCAL(0),
+                           MachineRepresentation::kWord32));
+  CHECK_EQ(10, r.Call(10));
+  CHECK_EQ(20, r.builder().ReadMemory(&memory[0]));
+}
 }  // namespace test_run_wasm_atomics
 }  // namespace wasm
 }  // namespace internal