commit b66846515656398af7af798172bcd8acd1d4ffa6
author Goran Jakovljevic <Goran.Jakovljevic@imgtec.com> Wed Jan 11 13:42:38 2017 +0100
committer Goran Jakovljevic <Goran.Jakovljevic@imgtec.com> Thu Jan 12 11:26:04 2017 +0100
tree c43958914f8679f264796c91717b8cb277d91d80
parent 1b8de86a54b04a56df97212ca584ff7188d32065

MIPS32: Change java.lang.Math.abs(float/double) intrinsics

abs.s and abs.d instructions can't be used when NAN2008=0 (R2 and
before). In these cases, abs(NaN) might change other bits (not just
sign bit).

This fixes test failure 631-checker-fp-abs for MIPS32.

Bug: 30758343
Test: mma test-art-target-run-test on CI20

Change-Id: I352efd66f2f72740406462c9ccc3bc4b1b5aea20

compiler/optimizing/intrinsics_mips.cc

diff --git a/compiler/optimizing/intrinsics_mips.cc b/compiler/optimizing/intrinsics_mips.cc
index e9c6615..cda3185 100644
--- a/compiler/optimizing/intrinsics_mips.cc
+++ b/compiler/optimizing/intrinsics_mips.cc
@@ -744,14 +744,54 @@
   GenBitCount(invoke->GetLocations(), Primitive::kPrimLong, IsR6(), GetAssembler());
 }
 
-static void MathAbsFP(LocationSummary* locations, bool is64bit, MipsAssembler* assembler) {
+static void MathAbsFP(LocationSummary* locations,
+                      bool is64bit,
+                      bool isR2OrNewer,
+                      bool isR6,
+                      MipsAssembler* assembler) {
   FRegister in = locations->InAt(0).AsFpuRegister<FRegister>();
   FRegister out = locations->Out().AsFpuRegister<FRegister>();
 
-  if (is64bit) {
-    __ AbsD(out, in);
+  // As a "quality of implementation", rather than pure "spec compliance", it is required that
+  // Math.abs() clears the sign bit (but changes nothing else) for all numbers, including NaN.
+  //
+  // The ABS.fmt instructions (abs.s and abs.d) do exactly that when NAN2008=1 (R6). For this case,
+  // both regular floating point numbers and NAN values are treated alike, only the sign bit is
+  // affected by this instruction.
+  // But when NAN2008=0 (R2 and before), the ABS.fmt instructions can't be used. For this case, any
+  // NaN operand signals invalid operation. This means that other bits (not just sign bit) might be
+  // changed when doing abs(NaN). Because of that, we clear sign bit in a different way.
+  if (isR6) {
+    if (is64bit) {
+      __ AbsD(out, in);
+    } else {
+      __ AbsS(out, in);
+    }
   } else {
-    __ AbsS(out, in);
+    if (is64bit) {
+      if (in != out) {
+        __ MovD(out, in);
+      }
+      __ MoveFromFpuHigh(TMP, in);
+      // ins instruction is not available for R1.
+      if (isR2OrNewer) {
+        __ Ins(TMP, ZERO, 31, 1);
+      } else {
+        __ Sll(TMP, TMP, 1);
+        __ Srl(TMP, TMP, 1);
+      }
+      __ MoveToFpuHigh(TMP, out);
+    } else {
+      __ Mfc1(TMP, in);
+      // ins instruction is not available for R1.
+      if (isR2OrNewer) {
+        __ Ins(TMP, ZERO, 31, 1);
+      } else {
+        __ Sll(TMP, TMP, 1);
+        __ Srl(TMP, TMP, 1);
+      }
+      __ Mtc1(TMP, out);
+    }
   }
 }
 
@@ -761,7 +801,7 @@
 }
 
 void IntrinsicCodeGeneratorMIPS::VisitMathAbsDouble(HInvoke* invoke) {
-  MathAbsFP(invoke->GetLocations(), /* is64bit */ true, GetAssembler());
+  MathAbsFP(invoke->GetLocations(), /* is64bit */ true, IsR2OrNewer(), IsR6(), GetAssembler());
 }
 
 // float java.lang.Math.abs(float)
@@ -770,7 +810,7 @@
 }
 
 void IntrinsicCodeGeneratorMIPS::VisitMathAbsFloat(HInvoke* invoke) {
-  MathAbsFP(invoke->GetLocations(), /* is64bit */ false, GetAssembler());
+  MathAbsFP(invoke->GetLocations(), /* is64bit */ false, IsR2OrNewer(), IsR6(), GetAssembler());
 }
 
 static void GenAbsInteger(LocationSummary* locations, bool is64bit, MipsAssembler* assembler) {