[turbofan] Revert algorithm simplification in Math.hypot

When a fast path was added for Math.hypot, the algorithm was also
simplified. This simplification turns out to be incorrect in some rare
edge cases. This cl reverts back to the original algorithm and converts it to torque.

Original cl: https://chromium-review.googlesource.com/c/v8/v8/+/1684178

Bug: v8:9546
Change-Id: If4e21504732f46081a8de823f50f499917f1a20c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1725200
Commit-Queue: Georg Neis <neis@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63070}

BUILD.gn

@@ -2057,7 +2057,6 @@ v8_source_set("v8_base_without_compiler") {
     "src/builtins/builtins-internal.cc",
     "src/builtins/builtins-intl.cc",
     "src/builtins/builtins-json.cc",
-    "src/builtins/builtins-math.cc",
     "src/builtins/builtins-number.cc",
     "src/builtins/builtins-object.cc",
     "src/builtins/builtins-promise.cc",

src/builtins/base.tq

@@ -1648,6 +1648,12 @@ extern operator '>=' macro BranchIfNumberGreaterThanOrEqual(
     Number, Number): never
     labels Taken, NotTaken;
 
+extern macro BranchIfFloat64IsNaN(float64): never
+    labels Taken, NotTaken;
+macro Float64IsNaN(n: float64): bool {
+  return (BranchIfFloat64IsNaN(n)) ? true : false;
+}
+
 // The type of all tagged values that can safely be compared with WordEqual.
 type TaggedWithIdentity =
     JSReceiver | FixedArrayBase | Oddball | Map | EmptyString;
@@ -1726,6 +1732,8 @@ extern operator '!=' macro Word32NotEqual(bool, bool): bool;
 
 extern operator '+' macro Float64Add(float64, float64): float64;
 extern operator '-' macro Float64Sub(float64, float64): float64;
+extern operator '*' macro Float64Mul(float64, float64): float64;
+extern operator '/' macro Float64Div(float64, float64): float64;
 
 extern operator '+' macro NumberAdd(Number, Number): Number;
 extern operator '-' macro NumberSub(Number, Number): Number;
@@ -2478,6 +2486,8 @@ extern operator '.floats[]=' macro StoreFixedDoubleArrayElement(
     FixedDoubleArray, intptr, float64): void;
 extern operator '.floats[]=' macro StoreFixedDoubleArrayElementSmi(
     FixedDoubleArray, Smi, float64): void;
+extern operator '.floats[]' macro LoadFixedDoubleArrayElement(
+    FixedDoubleArray, intptr): float64;
 operator '[]=' macro StoreFixedDoubleArrayDirect(
     a: FixedDoubleArray, i: Smi, v: Number) {
   a.floats[i] = Convert<float64>(v);

src/builtins/builtins-definitions.h

@@ -644,8 +644,6 @@ namespace internal {
   TFJ(MathCeil, 1, kReceiver, kX)                                              \
   /* ES6 #sec-math.floor */                                                    \
   TFJ(MathFloor, 1, kReceiver, kX)                                             \
-  /* ES6 #sec-math.hypot */                                                    \
-  CPP(MathHypot)                                                               \
   /* ES6 #sec-math.imul */                                                     \
   TFJ(MathImul, 2, kReceiver, kX, kY)                                          \
   /* ES6 #sec-math.max */                                                      \

src/builtins/builtins-math.cc

-// Copyright 2016 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.
-
-#include "src/builtins/builtins-utils.h"
-#include "src/builtins/builtins.h"
-#include "src/logging/counters.h"
-#include "src/objects/objects-inl.h"
-
-namespace v8 {
-namespace internal {
-
-// -----------------------------------------------------------------------------
-// ES6 section 20.2.2 Function Properties of the Math Object
-
-// ES6 section 20.2.2.18 Math.hypot ( value1, value2, ...values )
-BUILTIN(MathHypot) {
-  HandleScope scope(isolate);
-  int const length = args.length() - 1;
-  if (length == 0) return Smi::kZero;
-  DCHECK_LT(0, length);
-  double max = 0;
-  std::vector<double> abs_values;
-  abs_values.reserve(length);
-  for (int i = 0; i < length; i++) {
-    Handle<Object> x = args.at(i + 1);
-    ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, x,
-                                       Object::ToNumber(isolate, x));
-    double abs_value = std::abs(x->Number());
-    abs_values.push_back(abs_value);
-    // Use negation here to make sure that {max} is NaN
-    // in the end in case any of the arguments was NaN.
-    if (!(abs_value <= max)) {
-      max = abs_value;
-    }
-  }
-
-  if (max == 0) {
-    return Smi::kZero;
-  } else if (max == V8_INFINITY) {
-    return ReadOnlyRoots(isolate).infinity_value();
-  }
-  DCHECK(!(max <= 0));
-
-  // Kahan summation to avoid rounding errors.
-  // Normalize the numbers to the largest one to avoid overflow.
-  double sum = 0;
-  double compensation = 0;
-  for (int i = 0; i < length; i++) {
-    double n = abs_values[i] / max;
-    double summand = n * n - compensation;
-    double preliminary = sum + summand;
-    compensation = (preliminary - sum) - summand;
-    sum = preliminary;
-  }
-
-  return *isolate->factory()->NewNumber(std::sqrt(sum) * max);
-}
-
-}  // namespace internal
-}  // namespace v8

src/builtins/math.tq

@@ -235,4 +235,52 @@ namespace math {
     const value = Convert<float64>(ToNumber_Inline(context, x));
     return Convert<Number>(Float64Tanh(value));
   }
+
+  extern macro Float64Abs(float64): float64;
+
+  // ES6 #sec-math.hypot
+  transitioning javascript builtin
+  MathHypot(js-implicit context: Context, receiver: Object)(...arguments):
+      Number {
+    const length = arguments.length;
+    if (length == 0) {
+      return 0;
+    }
+    const absValues = AllocateZeroedFixedDoubleArray(length);
+    let oneArgIsNaN: bool = false;
+    let max: float64 = 0;
+    for (let i: intptr = 0; i < length; ++i) {
+      const value = Convert<float64>(ToNumber_Inline(context, arguments[i]));
+      if (Float64IsNaN(value)) {
+        oneArgIsNaN = true;
+      } else {
+        const absValue = Float64Abs(value);
+        absValues.floats[i] = absValue;
+        if (absValue > max) {
+          max = absValue;
+        }
+      }
+    }
+    if (max == V8_INFINITY) {
+      return V8_INFINITY;
+    } else if (oneArgIsNaN) {
+      return kNaN;
+    } else if (max == 0) {
+      return 0;
+    }
+    assert(max > 0);
+
+    // Kahan summation to avoid rounding errors.
+    // Normalize the numbers to the largest one to avoid overflow.
+    let sum: float64 = 0;
+    let compensation: float64 = 0;
+    for (let i: intptr = 0; i < length; ++i) {
+      const n = absValues.floats[i] / max;
+      const summand = n * n - compensation;
+      const preliminary = sum + summand;
+      compensation = (preliminary - sum) - summand;
+      sum = preliminary;
+    }
+    return Convert<Number>(Float64Sqrt(sum) * max);
+  }
 }

src/codegen/code-stub-assembler.h

@@ -1188,9 +1188,9 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
                                        SMI_PARAMETERS, if_hole);
   }
 
-  Node* LoadFixedDoubleArrayElement(TNode<FixedDoubleArray> object,
-                                    TNode<IntPtrT> index,
-                                    Label* if_hole = nullptr) {
+  TNode<Float64T> LoadFixedDoubleArrayElement(TNode<FixedDoubleArray> object,
+                                              TNode<IntPtrT> index,
+                                              Label* if_hole = nullptr) {
     return LoadFixedDoubleArrayElement(object, index, MachineType::Float64(), 0,
                                        INTPTR_PARAMETERS, if_hole);
   }

src/compiler/js-call-reducer.cc

@@ -179,100 +179,6 @@ Reduction JSCallReducer::ReduceMathMinMax(Node* node, const Operator* op,
   return Replace(value);
 }
 
-// ES section #sec-math.hypot Math.hypot ( value1, value2, ...values )
-Reduction JSCallReducer::ReduceMathHypot(Node* node) {
-  CallParameters const& p = CallParametersOf(node->op());
-  if (p.speculation_mode() == SpeculationMode::kDisallowSpeculation) {
-    return NoChange();
-  }
-  if (node->op()->ValueInputCount() < 3) {
-    Node* value = jsgraph()->ZeroConstant();
-    ReplaceWithValue(node, value);
-    return Replace(value);
-  }
-  Node* effect = NodeProperties::GetEffectInput(node);
-  Node* control = NodeProperties::GetControlInput(node);
-  NodeVector values(graph()->zone());
-
-  Node* max = effect =
-      graph()->NewNode(simplified()->SpeculativeToNumber(
-                           NumberOperationHint::kNumberOrOddball, p.feedback()),
-                       NodeProperties::GetValueInput(node, 2), effect, control);
-  max = graph()->NewNode(simplified()->NumberAbs(), max);
-  values.push_back(max);
-  for (int i = 3; i < node->op()->ValueInputCount(); ++i) {
-    Node* input = effect = graph()->NewNode(
-        simplified()->SpeculativeToNumber(NumberOperationHint::kNumberOrOddball,
-                                          p.feedback()),
-        NodeProperties::GetValueInput(node, i), effect, control);
-    input = graph()->NewNode(simplified()->NumberAbs(), input);
-    values.push_back(input);
-
-    // Make sure {max} is NaN in the end in case any argument was NaN.
-    max = graph()->NewNode(
-        common()->Select(MachineRepresentation::kTagged),
-        graph()->NewNode(simplified()->NumberLessThanOrEqual(), input, max),
-        max, input);
-  }
-
-  Node* check0 = graph()->NewNode(simplified()->NumberEqual(), max,
-                                  jsgraph()->ZeroConstant());
-  Node* branch0 =
-      graph()->NewNode(common()->Branch(BranchHint::kFalse), check0, control);
-
-  Node* if_true0 = graph()->NewNode(common()->IfTrue(), branch0);
-  Node* vtrue0 = jsgraph()->ZeroConstant();
-
-  Node* if_false0 = graph()->NewNode(common()->IfFalse(), branch0);
-  Node* vfalse0;
-  {
-    Node* check1 = graph()->NewNode(simplified()->NumberEqual(), max,
-                                    jsgraph()->Constant(V8_INFINITY));
-    Node* branch1 = graph()->NewNode(common()->Branch(BranchHint::kFalse),
-                                     check1, if_false0);
-
-    Node* if_true1 = graph()->NewNode(common()->IfTrue(), branch1);
-    Node* vtrue1 = jsgraph()->Constant(V8_INFINITY);
-
-    Node* if_false1 = graph()->NewNode(common()->IfFalse(), branch1);
-    Node* vfalse1;
-    {
-      // Kahan summation to avoid rounding errors.
-      // Normalize the numbers to the largest one to avoid overflow.
-      Node* sum = jsgraph()->ZeroConstant();
-      Node* compensation = jsgraph()->ZeroConstant();
-      for (Node* value : values) {
-        Node* n = graph()->NewNode(simplified()->NumberDivide(), value, max);
-        Node* summand = graph()->NewNode(
-            simplified()->NumberSubtract(),
-            graph()->NewNode(simplified()->NumberMultiply(), n, n),
-            compensation);
-        Node* preliminary =
-            graph()->NewNode(simplified()->NumberAdd(), sum, summand);
-        compensation = graph()->NewNode(
-            simplified()->NumberSubtract(),
-            graph()->NewNode(simplified()->NumberSubtract(), preliminary, sum),
-            summand);
-        sum = preliminary;
-      }
-      vfalse1 = graph()->NewNode(
-          simplified()->NumberMultiply(),
-          graph()->NewNode(simplified()->NumberSqrt(), sum), max);
-    }
-
-    if_false0 = graph()->NewNode(common()->Merge(2), if_true1, if_false1);
-    vfalse0 = graph()->NewNode(common()->Phi(MachineRepresentation::kTagged, 2),
-                               vtrue1, vfalse1, if_false0);
-  }
-
-  control = graph()->NewNode(common()->Merge(2), if_true0, if_false0);
-  Node* value =
-      graph()->NewNode(common()->Phi(MachineRepresentation::kTagged, 2), vtrue0,
-                       vfalse0, control);
-  ReplaceWithValue(node, value, effect, control);
-  return Replace(value);
-}
-
 Reduction JSCallReducer::Reduce(Node* node) {
   switch (node->opcode()) {
     case IrOpcode::kJSConstruct:
@@ -3669,8 +3575,6 @@ Reduction JSCallReducer::ReduceJSCall(Node* node,
       return ReduceMathUnary(node, simplified()->NumberFloor());
     case Builtins::kMathFround:
       return ReduceMathUnary(node, simplified()->NumberFround());
-    case Builtins::kMathHypot:
-      return ReduceMathHypot(node);
     case Builtins::kMathLog:
       return ReduceMathUnary(node, simplified()->NumberLog());
     case Builtins::kMathLog1p:

src/compiler/js-call-reducer.h

@@ -156,7 +156,6 @@ class V8_EXPORT_PRIVATE JSCallReducer final : public AdvancedReducer {
   Reduction ReduceMathImul(Node* node);
   Reduction ReduceMathClz32(Node* node);
   Reduction ReduceMathMinMax(Node* node, const Operator* op, Node* empty_value);
-  Reduction ReduceMathHypot(Node* node);
 
   Reduction ReduceNumberIsFinite(Node* node);
   Reduction ReduceNumberIsInteger(Node* node);

src/compiler/typer.cc

@@ -1560,6 +1560,7 @@ Type Typer::Visitor::JSCallTyper(Type fun, Typer* t) {
     case Builtins::kMathPow:
     case Builtins::kMathMax:
     case Builtins::kMathMin:
+    case Builtins::kMathHypot:
       return Type::Number();
     case Builtins::kMathImul:
       return Type::Signed32();

test/mjsunit/regress/regress-9546.js

+// Copyright 2019 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.
+//
+// Flags: --allow-natives-syntax
+
+// Sanity checks.
+assertEquals(Math.hypot(3, 4), 5);
+assertEquals(Math.hypot(1, 2, 3, 4, 5, 27), 28);
+
+// Regress.
+assertEquals(Math.hypot(Infinity, NaN), Infinity);
+assertEquals(Math.hypot(NaN, 0), NaN);
+assertEquals(Math.hypot(NaN, Infinity), Infinity);
+assertEquals(Math.hypot(0, NaN), NaN);
+assertEquals(Math.hypot(NaN, 1, 2, 3, 4, 5, 0), NaN);
+assertEquals(Math.hypot(NaN, Infinity, 2, 3, 4, 5, 0), Infinity);
+
+// Verify optimized code works as intended.
+function two_hypot(a, b) {
+  return Math.hypot(a, b);
+}
+
+function six_hypot(a, b, c, d, e, f) {
+  return Math.hypot(a, b, c, d, e, f);
+}
+
+%PrepareFunctionForOptimization(two_hypot);
+two_hypot(1, 2);
+two_hypot(3, 4);
+two_hypot(5, 6);
+%OptimizeFunctionOnNextCall(two_hypot);
+assertEquals(two_hypot(3, 4), 5);
+
+// Regress 2 parameter case.
+assertEquals(two_hypot(Infinity, NaN), Infinity);
+assertEquals(two_hypot(NaN, 0), NaN);
+assertEquals(two_hypot(NaN, Infinity), Infinity);
+assertEquals(two_hypot(0, NaN), NaN);
+
+// Regress many parameters case.
+%PrepareFunctionForOptimization(six_hypot);
+six_hypot(1, 2, 3, 4, 5, 6);
+six_hypot(3, 4, 5, 6, 7, 8);
+six_hypot(5, 6, 7, 8, 9, 10);
+%OptimizeFunctionOnNextCall(six_hypot);
+assertEquals(six_hypot(1, 2, 3, 4, 5, 27), 28);
+
+assertEquals(six_hypot(0, 0, 0, 0, 0, 0), 0);
+assertEquals(six_hypot(NaN, 1, 2, 3, 4, 5, 0), NaN);
+assertEquals(six_hypot(NaN, Infinity, 2, 3, 4, 5, 0), Infinity);
+assertEquals(six_hypot(1, 2, 3, 4, 5, NaN), NaN);
+assertEquals(six_hypot(Infinity, 2, 3, 4, 5, NaN), Infinity);