[wasm-simd][arm64] Do not emit Bic(x, imm) for AndNot(imm, x)

Fix bug where AndNot(x, imm) and AndNot(imm, x) both become Bic(x, imm).

Bug: chromium:1318092
Change-Id: I0ca2c65a1e5d64da0347c86346e7c4dc04943eff
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3613386Reviewed-by: Maya Lekova <mslekova@chromium.org>
Reviewed-by: Andreas Haas <ahaas@chromium.org>
Commit-Queue: George Wort <george.wort@arm.com>
Reviewed-by: Deepti Gandluri <gdeepti@chromium.org>
Cr-Commit-Position: refs/heads/main@{#80600}

src/compiler/backend/arm64/instruction-selector-arm64.cc

@@ -3670,10 +3670,14 @@ base::Optional<BicImmParam> BicImmConstHelper(Node* const_node, bool not_imm) {
   return BicImm32bitHelper(not_imm ? ~val[0] : val[0]);
 }
 
-base::Optional<BicImmResult> BicImmHelper(Node* or_node, bool not_imm) {
-  Node* left = or_node->InputAt(0);
-  Node* right = or_node->InputAt(1);
-  if (left->opcode() == IrOpcode::kS128Const) {
+base::Optional<BicImmResult> BicImmHelper(Node* and_node, bool not_imm) {
+  Node* left = and_node->InputAt(0);
+  Node* right = and_node->InputAt(1);
+  // If we are negating the immediate then we are producing And(x, imm), and so
+  // can take the immediate from the left or right input. Otherwise we are
+  // producing And(x, Not(imm)), which can only be used when the immediate is
+  // the right (negated) input.
+  if (not_imm && left->opcode() == IrOpcode::kS128Const) {
     return BicImmResult(BicImmConstHelper(left, not_imm), left, right);
   }
   if (right->opcode() == IrOpcode::kS128Const) {
@@ -3709,6 +3713,7 @@ void InstructionSelector::VisitS128AndNot(Node* node) {
 }
 
 void InstructionSelector::VisitS128And(Node* node) {
+  // AndNot can be used if we negate the immediate input of And.
   if (!TryEmitS128AndNotImm(this, node, true)) {
     VisitRRR(this, kArm64S128And, node);
   }

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

@@ -1545,53 +1545,72 @@ WASM_SIMD_TEST(S128And) {
                     [](int32_t x, int32_t y) { return x & y; });
 }
 
+enum ConstSide { kConstLeft, kConstRight };
+
 template <typename ScalarType>
 using BinOp = ScalarType (*)(ScalarType, ScalarType);
 template <typename ScalarType>
 void RunS128ConstBinOpTest(TestExecutionTier execution_tier,
-                           WasmOpcode binop_opcode, WasmOpcode splat_opcode,
+                           ConstSide const_side, WasmOpcode binop_opcode,
+                           WasmOpcode splat_opcode,
                            BinOp<ScalarType> expected_op) {
   for (ScalarType x : compiler::ValueHelper::GetVector<ScalarType>()) {
     WasmRunner<int32_t, ScalarType> r(execution_tier);
     // Global to hold output.
-    ScalarType* g1 = r.builder().template AddGlobal<ScalarType>(kWasmS128);
-    ScalarType* g2 = r.builder().template AddGlobal<ScalarType>(kWasmS128);
+    ScalarType* g = r.builder().template AddGlobal<ScalarType>(kWasmS128);
     // Build a function to splat one argument into a local,
     // and execute the op with a const as the second argument
     byte value = 0;
-    byte temp1 = r.AllocateLocal(kWasmS128);
+    byte temp = r.AllocateLocal(kWasmS128);
     uint8_t const_buffer[16];
     for (size_t i = 0; i < kSimd128Size / sizeof(ScalarType); i++) {
       WriteLittleEndianValue<ScalarType>(
           base::bit_cast<ScalarType*>(&const_buffer[0]) + i, x);
     }
-    BUILD(
-        r,
-        WASM_LOCAL_SET(temp1,
-                       WASM_SIMD_OPN(splat_opcode, WASM_LOCAL_GET(value))),
-        WASM_GLOBAL_SET(0, WASM_SIMD_BINOP(binop_opcode, WASM_LOCAL_GET(temp1),
-                                           WASM_SIMD_CONSTANT(const_buffer))),
-        WASM_GLOBAL_SET(
-            1, WASM_SIMD_BINOP(binop_opcode, WASM_SIMD_CONSTANT(const_buffer),
-                               WASM_LOCAL_GET(temp1))),
-        WASM_ONE);
+    switch (const_side) {
+      case kConstLeft:
+        BUILD(
+            r,
+            WASM_LOCAL_SET(temp,
+                           WASM_SIMD_OPN(splat_opcode, WASM_LOCAL_GET(value))),
+            WASM_GLOBAL_SET(0, WASM_SIMD_BINOP(binop_opcode,
+                                               WASM_SIMD_CONSTANT(const_buffer),
+                                               WASM_LOCAL_GET(temp))),
+            WASM_ONE);
+        break;
+      case kConstRight:
+        BUILD(r,
+              WASM_LOCAL_SET(
+                  temp, WASM_SIMD_OPN(splat_opcode, WASM_LOCAL_GET(value))),
+              WASM_GLOBAL_SET(
+                  0, WASM_SIMD_BINOP(binop_opcode, WASM_LOCAL_GET(temp),
+                                     WASM_SIMD_CONSTANT(const_buffer))),
+              WASM_ONE);
+        break;
+    }
     for (ScalarType y : compiler::ValueHelper::GetVector<ScalarType>()) {
       r.Call(y);
-      ScalarType expected1 = expected_op(y, x);
-      ScalarType expected2 = expected_op(x, y);
+      ScalarType expected =
+          (const_side == kConstLeft) ? expected_op(x, y) : expected_op(y, x);
       for (size_t i = 0; i < kSimd128Size / sizeof(ScalarType); i++) {
-        CHECK_EQ(expected1, LANE(g1, i));
-        CHECK_EQ(expected2, LANE(g2, i));
+        CHECK_EQ(expected, LANE(g, i));
       }
     }
   }
 }
 
 WASM_SIMD_TEST(S128AndImm) {
-  RunS128ConstBinOpTest<int32_t>(execution_tier, kExprS128And, kExprI32x4Splat,
+  RunS128ConstBinOpTest<int32_t>(execution_tier, kConstLeft, kExprS128And,
+                                 kExprI32x4Splat,
+                                 [](int32_t x, int32_t y) { return x & y; });
+  RunS128ConstBinOpTest<int32_t>(execution_tier, kConstRight, kExprS128And,
+                                 kExprI32x4Splat,
                                  [](int32_t x, int32_t y) { return x & y; });
   RunS128ConstBinOpTest<int16_t>(
-      execution_tier, kExprS128And, kExprI16x8Splat,
+      execution_tier, kConstLeft, kExprS128And, kExprI16x8Splat,
+      [](int16_t x, int16_t y) { return static_cast<int16_t>(x & y); });
+  RunS128ConstBinOpTest<int16_t>(
+      execution_tier, kConstRight, kExprS128And, kExprI16x8Splat,
       [](int16_t x, int16_t y) { return static_cast<int16_t>(x & y); });
 }
 
@@ -1612,11 +1631,17 @@ WASM_SIMD_TEST(S128AndNot) {
 }
 
 WASM_SIMD_TEST(S128AndNotImm) {
-  RunS128ConstBinOpTest<int32_t>(execution_tier, kExprS128AndNot,
+  RunS128ConstBinOpTest<int32_t>(execution_tier, kConstLeft, kExprS128AndNot,
+                                 kExprI32x4Splat,
+                                 [](int32_t x, int32_t y) { return x & ~y; });
+  RunS128ConstBinOpTest<int32_t>(execution_tier, kConstRight, kExprS128AndNot,
                                  kExprI32x4Splat,
                                  [](int32_t x, int32_t y) { return x & ~y; });
   RunS128ConstBinOpTest<int16_t>(
-      execution_tier, kExprS128AndNot, kExprI16x8Splat,
+      execution_tier, kConstLeft, kExprS128AndNot, kExprI16x8Splat,
+      [](int16_t x, int16_t y) { return static_cast<int16_t>(x & ~y); });
+  RunS128ConstBinOpTest<int16_t>(
+      execution_tier, kConstRight, kExprS128AndNot, kExprI16x8Splat,
       [](int16_t x, int16_t y) { return static_cast<int16_t>(x & ~y); });
 }
 

test/unittests/compiler/arm64/instruction-selector-arm64-unittest.cc

@@ -5575,6 +5575,7 @@ struct SIMDConstAndTest {
   const uint8_t data[16];
   const Operator* (MachineOperatorBuilder::*simd_op)();
   const ArchOpcode expected_op;
+  const bool symmetrical;
   const uint8_t lane_size;
   const uint8_t shift_amount;
   const int32_t expected_imm;
@@ -5586,6 +5587,7 @@ static const SIMDConstAndTest SIMDConstAndTests[] = {
       0xFF, 0xFE, 0xFF, 0xFE},
      &MachineOperatorBuilder::S128And,
      kArm64S128AndNot,
+     true,
      16,
      8,
      0x01,
@@ -5594,6 +5596,7 @@ static const SIMDConstAndTest SIMDConstAndTests[] = {
       0xFE, 0xFF, 0xFE, 0xFF},
      &MachineOperatorBuilder::S128And,
      kArm64S128AndNot,
+     true,
      16,
      0,
      0x01,
@@ -5603,6 +5606,7 @@ static const SIMDConstAndTest SIMDConstAndTests[] = {
       0xFF, 0xFF, 0xFF, 0xFE},
      &MachineOperatorBuilder::S128And,
      kArm64S128AndNot,
+     true,
      32,
      24,
      0x01,
@@ -5611,6 +5615,7 @@ static const SIMDConstAndTest SIMDConstAndTests[] = {
       0xFF, 0xFF, 0xFE, 0xFF},
      &MachineOperatorBuilder::S128And,
      kArm64S128AndNot,
+     true,
      32,
      16,
      0x01,
@@ -5619,6 +5624,7 @@ static const SIMDConstAndTest SIMDConstAndTests[] = {
       0xFF, 0xFE, 0xFF, 0xFF},
      &MachineOperatorBuilder::S128And,
      kArm64S128AndNot,
+     true,
      32,
      8,
      0x01,
@@ -5627,6 +5633,7 @@ static const SIMDConstAndTest SIMDConstAndTests[] = {
       0xFE, 0xFF, 0xFF, 0xFF},
      &MachineOperatorBuilder::S128And,
      kArm64S128AndNot,
+     true,
      32,
      0,
      0x01,
@@ -5636,6 +5643,7 @@ static const SIMDConstAndTest SIMDConstAndTests[] = {
       0xEE, 0xEE, 0xEE, 0xEE},
      &MachineOperatorBuilder::S128And,
      kArm64S128And,
+     true,
      0,
      0,
      0x00,
@@ -5645,6 +5653,7 @@ static const SIMDConstAndTest SIMDConstAndTests[] = {
       0x00, 0x01, 0x00, 0x01},
      &MachineOperatorBuilder::S128AndNot,
      kArm64S128AndNot,
+     false,
      16,
      8,
      0x01,
@@ -5653,6 +5662,7 @@ static const SIMDConstAndTest SIMDConstAndTests[] = {
       0x01, 0x00, 0x01, 0x00},
      &MachineOperatorBuilder::S128AndNot,
      kArm64S128AndNot,
+     false,
      16,
      0,
      0x01,
@@ -5662,6 +5672,7 @@ static const SIMDConstAndTest SIMDConstAndTests[] = {
       0x00, 0x00, 0x00, 0x01},
      &MachineOperatorBuilder::S128AndNot,
      kArm64S128AndNot,
+     false,
      32,
      24,
      0x01,
@@ -5670,6 +5681,7 @@ static const SIMDConstAndTest SIMDConstAndTests[] = {
       0x00, 0x00, 0x01, 0x00},
      &MachineOperatorBuilder::S128AndNot,
      kArm64S128AndNot,
+     false,
      32,
      16,
      0x01,
@@ -5678,6 +5690,7 @@ static const SIMDConstAndTest SIMDConstAndTests[] = {
       0x00, 0x01, 0x00, 0x00},
      &MachineOperatorBuilder::S128AndNot,
      kArm64S128AndNot,
+     false,
      32,
      8,
      0x01,
@@ -5686,6 +5699,7 @@ static const SIMDConstAndTest SIMDConstAndTests[] = {
       0x01, 0x00, 0x00, 0x00},
      &MachineOperatorBuilder::S128AndNot,
      kArm64S128AndNot,
+     false,
      32,
      0,
      0x01,
@@ -5695,6 +5709,7 @@ static const SIMDConstAndTest SIMDConstAndTests[] = {
       0xEE, 0xEE, 0xEE, 0xEE},
      &MachineOperatorBuilder::S128AndNot,
      kArm64S128AndNot,
+     false,
      0,
      0,
      0x00,
@@ -5713,8 +5728,11 @@ TEST_P(InstructionSelectorSIMDConstAndTest, ConstAnd) {
     Node* op = m.AddNode((m.machine()->*param.simd_op)(), cnst, m.Parameter(0));
     m.Return(op);
     Stream s = m.Build();
-    ASSERT_EQ(param.size, s.size());
-    if (param.size == 1) {
+
+    // Bic cannot always be applied when the immediate is on the left
+    size_t expected_size = param.symmetrical ? param.size : 2;
+    ASSERT_EQ(expected_size, s.size());
+    if (expected_size == 1) {
       EXPECT_EQ(param.expected_op, s[0]->arch_opcode());
       EXPECT_EQ(3U, s[0]->InputCount());
       EXPECT_EQ(1U, s[0]->OutputCount());