[WASM SIMD] Implement horizontal add for float and integer types.

src/compiler/arm/code-generator-arm.cc

Index: src/compiler/arm/code-generator-arm.cc
diff --git a/src/compiler/arm/code-generator-arm.cc b/src/compiler/arm/code-generator-arm.cc
index 5e916f2352a0c16593540c0aaaff7b437e3ae769..bc2f2985a70ce7697bc9c7644e21344f53ecf621 100644
--- a/src/compiler/arm/code-generator-arm.cc
+++ b/src/compiler/arm/code-generator-arm.cc
@@ -1611,6 +1611,24 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
               i.InputSimd128Register(1));
       break;
     }
+    case kArmF32x4AddHoriz: {
+      Simd128Register dst = i.OutputSimd128Register(),
+                      src0 = i.InputSimd128Register(0),
+                      src1 = i.InputSimd128Register(1);
+      // Make sure we don't overwrite source data before it's used.
+      if (dst.is(src0)) {
+        __ vpadd(dst.low(), src0.low(), src0.high());

[georgia.kouveli, 2017/04/20 14:53:06]

The inputs to all the vpadd instructions seem to be in the wrong order,
shouldn't it be high first and low second? (VPADD is not commutative).

The RunF32x4HorizOpTest and other tests will not catch this, as the two inputs
have splatted values, which means the two pairs in dst.high() will have the same
result and the two pairs in dst.low() will also have the same result. It would
help to update the test to have a different expected value in each pair, this
way you would catch any such issues.

[bbudge, 2017/04/21 20:18:58]

I changed the tests to use the permutation test machinery, which assigns unique
(integral) values to each lane of each vector. This eliminates a lot of code and
results in better tests (no more splatted inputs.)

I think there's some confusion in the assembler calls - vpadd stores the first
source operand as Vn, and the second source operand as Vm, which is flipped from
the normal assembler syntax. I'm not sure what to do about that though, as this
matches a lot of existing NEON assembler code. Perhaps another CL to straighten
out the confusion?

+        if (dst.is(src1)) {
+          __ vmov(dst.high(), dst.low());
+        } else {
+          __ vpadd(dst.high(), src1.low(), src1.high());
+        }
+      } else {
+        __ vpadd(dst.high(), src1.low(), src1.high());
+        __ vpadd(dst.low(), src0.low(), src0.high());
+      }
+      break;
+    }
     case kArmF32x4Sub: {
       __ vsub(i.OutputSimd128Register(), i.InputSimd128Register(0),
               i.InputSimd128Register(1));
@@ -1709,6 +1727,24 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
               i.InputSimd128Register(1));
       break;
     }
+    case kArmI32x4AddHoriz: {
+      Simd128Register dst = i.OutputSimd128Register(),
+                      src0 = i.InputSimd128Register(0),
+                      src1 = i.InputSimd128Register(1);
+      // Make sure we don't overwrite source data before it's used.

[georgia.kouveli, 2017/04/20 14:53:06]

Might make sense to factor out this code, since it's repeated for all types.

[bbudge, 2017/04/21 20:18:58]

Done. (And I did the same for the repetitious narrowing integer conversions
too.)

+      if (dst.is(src0)) {
+        __ vpadd(Neon32, dst.low(), src0.low(), src0.high());
+        if (dst.is(src1)) {
+          __ vmov(dst.high(), dst.low());
+        } else {
+          __ vpadd(Neon32, dst.high(), src1.low(), src1.high());
+        }
+      } else {
+        __ vpadd(Neon32, dst.high(), src1.low(), src1.high());
+        __ vpadd(Neon32, dst.low(), src0.low(), src0.high());
+      }
+      break;
+    }
     case kArmI32x4Sub: {
       __ vsub(Neon32, i.OutputSimd128Register(), i.InputSimd128Register(0),
               i.InputSimd128Register(1));
@@ -1857,6 +1893,24 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
                i.InputSimd128Register(1));
       break;
     }
+    case kArmI16x8AddHoriz: {
+      Simd128Register dst = i.OutputSimd128Register(),
+                      src0 = i.InputSimd128Register(0),
+                      src1 = i.InputSimd128Register(1);
+      // Make sure we don't overwrite source data before it's used.
+      if (dst.is(src0)) {
+        __ vpadd(Neon16, dst.low(), src0.low(), src0.high());
+        if (dst.is(src1)) {
+          __ vmov(dst.high(), dst.low());
+        } else {
+          __ vpadd(Neon16, dst.high(), src1.low(), src1.high());
+        }
+      } else {
+        __ vpadd(Neon16, dst.high(), src1.low(), src1.high());
+        __ vpadd(Neon16, dst.low(), src0.low(), src0.high());
+      }
+      break;
+    }
     case kArmI16x8Sub: {
       __ vsub(Neon16, i.OutputSimd128Register(), i.InputSimd128Register(0),
               i.InputSimd128Register(1));
@@ -2025,6 +2079,24 @@ CodeGenerator::CodeGenResult CodeGenerator::AssembleArchInstruction(
                i.InputSimd128Register(1));
       break;
     }
+    case kArmI8x16AddHoriz: {
+      Simd128Register dst = i.OutputSimd128Register(),
+                      src0 = i.InputSimd128Register(0),
+                      src1 = i.InputSimd128Register(1);
+      // Make sure we don't overwrite source data before it's used.
+      if (dst.is(src0)) {
+        __ vpadd(Neon8, dst.low(), src0.low(), src0.high());
+        if (dst.is(src1)) {
+          __ vmov(dst.high(), dst.low());
+        } else {
+          __ vpadd(Neon8, dst.high(), src1.low(), src1.high());
+        }
+      } else {
+        __ vpadd(Neon8, dst.high(), src1.low(), src1.high());
+        __ vpadd(Neon8, dst.low(), src0.low(), src0.high());
+      }
+      break;
+    }
     case kArmI8x16Sub: {
       __ vsub(Neon8, i.OutputSimd128Register(), i.InputSimd128Register(0),
               i.InputSimd128Register(1));