[arm][arm64] Minor improvements to MathMaxMin.

BUG=

Review URL: https://codereview.chromium.org/1652023002

Cr-Commit-Position: refs/heads/master@{#33666}

src/arm/builtins-arm.cc

@@ -155,7 +155,9 @@ void Builtins::Generate_MathMaxMin(MacroAssembler* masm, MathMaxMinKind kind) {
   // +Infinity), with the tagged value in r1 and the double value in d1.
   __ LoadRoot(r1, root_index);
   __ vldr(d1, FieldMemOperand(r1, HeapNumber::kValueOffset));
-  __ mov(r4, r0);
+
+  // Remember how many slots to drop (including the receiver).
+  __ add(r4, r0, Operand(1));
 
   Label done_loop, loop;
   __ bind(&loop);
@@ -177,8 +179,8 @@ void Builtins::Generate_MathMaxMin(MacroAssembler* masm, MathMaxMinKind kind) {
     {
       // Parameter is not a Number, use the ToNumberStub to convert it.
       FrameAndConstantPoolScope scope(masm, StackFrame::INTERNAL);
-      __ SmiTag(r0, r0);
-      __ SmiTag(r4, r4);
+      __ SmiTag(r0);
+      __ SmiTag(r4);
       __ Push(r0, r1, r4);
       __ mov(r0, r2);
       ToNumberStub stub(masm->isolate());
@@ -187,12 +189,10 @@ void Builtins::Generate_MathMaxMin(MacroAssembler* masm, MathMaxMinKind kind) {
       __ Pop(r0, r1, r4);
       {
         // Restore the double accumulator value (d1).
-        Label restore_smi, done_restore;
-        __ JumpIfSmi(r1, &restore_smi);
-        __ vldr(d1, FieldMemOperand(r1, HeapNumber::kValueOffset));
-        __ b(&done_restore);
-        __ bind(&restore_smi);
+        Label done_restore;
         __ SmiToDouble(d1, r1);
+        __ JumpIfSmi(r1, &done_restore);
+        __ vldr(d1, FieldMemOperand(r1, HeapNumber::kValueOffset));
         __ bind(&done_restore);
       }
       __ SmiUntag(r4);
@@ -235,7 +235,7 @@ void Builtins::Generate_MathMaxMin(MacroAssembler* masm, MathMaxMinKind kind) {
   __ bind(&done_loop);
   __ mov(r0, r1);
   __ Drop(r4);
-  __ Ret(1);
+  __ Ret();
 }
 
 // static

src/arm64/builtins-arm64.cc

@@ -147,18 +147,17 @@ void Builtins::Generate_MathMaxMin(MacroAssembler* masm, MathMaxMinKind kind) {
   // -----------------------------------
   ASM_LOCATION("Builtins::Generate_MathMaxMin");
 
-  Condition const cc_done = (kind == MathMaxMinKind::kMin) ? mi : gt;
-  Condition const cc_swap = (kind == MathMaxMinKind::kMin) ? gt : mi;
   Heap::RootListIndex const root_index =
       (kind == MathMaxMinKind::kMin) ? Heap::kInfinityValueRootIndex
                                      : Heap::kMinusInfinityValueRootIndex;
-  DoubleRegister const reg = (kind == MathMaxMinKind::kMin) ? d2 : d1;
 
   // Load the accumulator with the default return value (either -Infinity or
   // +Infinity), with the tagged value in x1 and the double value in d1.
   __ LoadRoot(x1, root_index);
   __ Ldr(d1, FieldMemOperand(x1, HeapNumber::kValueOffset));
-  __ Mov(x4, x0);
+
+  // Remember how many slots to drop (including the receiver).
+  __ Add(x4, x0, 1);
 
   Label done_loop, loop;
   __ Bind(&loop);
@@ -172,11 +171,9 @@ void Builtins::Generate_MathMaxMin(MacroAssembler* masm, MathMaxMinKind kind) {
 
     // Load the double value of the parameter into d2, maybe converting the
     // parameter to a number first using the ToNumberStub if necessary.
-    Label convert, convert_smi, convert_number, done_convert;
-    __ Bind(&convert);
+    Label convert_smi, convert_number, done_convert;
     __ JumpIfSmi(x2, &convert_smi);
-    __ Ldr(x3, FieldMemOperand(x2, HeapObject::kMapOffset));
-    __ JumpIfRoot(x3, Heap::kHeapNumberMapRootIndex, &convert_number);
+    __ JumpIfHeapNumber(x2, &convert_number);
     {
       // Parameter is not a Number, use the ToNumberStub to convert it.
       FrameScope scope(masm, StackFrame::INTERNAL);
@@ -190,54 +187,44 @@ void Builtins::Generate_MathMaxMin(MacroAssembler* masm, MathMaxMinKind kind) {
       __ Pop(x4, x1, x0);
       {
         // Restore the double accumulator value (d1).
-        Label restore_smi, done_restore;
-        __ JumpIfSmi(x1, &restore_smi);
+        Label done_restore;
+        __ SmiUntagToDouble(d1, x1, kSpeculativeUntag);
+        __ JumpIfSmi(x1, &done_restore);
         __ Ldr(d1, FieldMemOperand(x1, HeapNumber::kValueOffset));
-        __ B(&done_restore);
-        __ Bind(&restore_smi);
-        __ SmiUntagToDouble(d1, x1);
-        __ bind(&done_restore);
+        __ Bind(&done_restore);
       }
       __ SmiUntag(x4);
       __ SmiUntag(x0);
     }
-    __ B(&convert);
+    __ AssertNumber(x2);
+    __ JumpIfSmi(x2, &convert_smi);
+
     __ Bind(&convert_number);
     __ Ldr(d2, FieldMemOperand(x2, HeapNumber::kValueOffset));
     __ B(&done_convert);
+
     __ Bind(&convert_smi);
     __ SmiUntagToDouble(d2, x2);
     __ Bind(&done_convert);
 
-    // Perform the actual comparison with the accumulator value on the left hand
-    // side (d1) and the next parameter value on the right hand side (d2).
-    Label compare_nan, compare_swap;
-    __ Fcmp(d1, d2);
-    __ B(cc_done, &loop);
-    __ B(cc_swap, &compare_swap);
-    __ B(vs, &compare_nan);
-
-    // Left and right hand side are equal, check for -0 vs. +0.
-    __ Fmov(x3, reg);
-    __ TestAndBranchIfAllClear(x3, V8_INT64_C(0x8000000000000000), &loop);
-
-    // Result is on the right hand side.
-    __ Bind(&compare_swap);
-    __ Fmov(d1, d2);
-    __ Mov(x1, x2);
-    __ B(&loop);
-
-    // At least one side is NaN, which means that the result will be NaN too.
-    __ Bind(&compare_nan);
-    __ LoadRoot(x1, Heap::kNanValueRootIndex);
-    __ Ldr(d1, FieldMemOperand(x1, HeapNumber::kValueOffset));
+    // We can use a single fmin/fmax for the operation itself, but we then need
+    // to work out which HeapNumber (or smi) the result came from.
+    __ Fmov(x11, d1);
+    if (kind == MathMaxMinKind::kMin) {
+      __ Fmin(d1, d1, d2);
+    } else {
+      DCHECK(kind == MathMaxMinKind::kMax);
+      __ Fmax(d1, d1, d2);
+    }
+    __ Fmov(x10, d1);
+    __ Cmp(x10, x11);
+    __ Csel(x1, x1, x2, eq);
     __ B(&loop);
   }
 
   __ Bind(&done_loop);
   __ Mov(x0, x1);
   __ Drop(x4);
-  __ Drop(1);
   __ Ret();
 }
 

src/arm64/macro-assembler-arm64.cc

@@ -1677,6 +1677,15 @@ void MacroAssembler::AssertPositiveOrZero(Register value) {
   }
 }
 
+void MacroAssembler::AssertNumber(Register value) {
+  if (emit_debug_code()) {
+    Label done;
+    JumpIfSmi(value, &done);
+    JumpIfHeapNumber(value, &done);
+    Abort(kOperandIsNotANumber);
+    Bind(&done);
+  }
+}
 
 void MacroAssembler::CallStub(CodeStub* stub, TypeFeedbackId ast_id) {
   DCHECK(AllowThisStubCall(stub));  // Stub calls are not allowed in some stubs.

src/arm64/macro-assembler-arm64.h

@@ -982,6 +982,9 @@ class MacroAssembler : public Assembler {
   // --debug-code.
   void AssertPositiveOrZero(Register value);
 
+  // Abort execution if argument is not a number (heap number or smi).
+  void AssertNumber(Register value);
+
   void JumpIfHeapNumber(Register object, Label* on_heap_number,
                         SmiCheckType smi_check_type = DONT_DO_SMI_CHECK);
   void JumpIfNotHeapNumber(Register object, Label* on_not_heap_number,