Bug 1526024 - Handle rcx as a destination register r=tcampbell
authorMatthew Gaudet <mgaudet@mozilla.com>
Wed, 27 Feb 2019 14:31:36 +0000
changeset 519325 b8712597ac579317f0ac45072cf007e654655be5
parent 519324 39bf0bf1a7443de823764af029b0e7755602e0c7
child 519326 46246cd080fa6c1278d967277bf91578ea94d70e
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcampbell
bugs1526024
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1526024 - Handle rcx as a destination register r=tcampbell Also add unit tests Differential Revision: https://phabricator.services.mozilla.com/D19077
js/src/jit/x86-shared/MacroAssembler-x86-shared-inl.h
js/src/jsapi-tests/testJitMacroAssembler.cpp
--- a/js/src/jit/x86-shared/MacroAssembler-x86-shared-inl.h
+++ b/js/src/jit/x86-shared/MacroAssembler-x86-shared-inl.h
@@ -303,38 +303,68 @@ void MacroAssembler::rotateRight(Registe
 // ===============================================================
 // Shift instructions
 
 void MacroAssembler::lshift32(Register shift, Register srcDest) {
   MOZ_ASSERT(shift == ecx);
   shll_cl(srcDest);
 }
 
-inline void FlexibleShift32(MacroAssembler& masm, Register shift, Register dest,
-                            bool left, bool arithmetic = false) {
-  if (shift != ecx) {
-    if (dest != ecx) {
-      masm.push(ecx);
-    }
-    masm.mov(shift, ecx);
+// All the shift instructions have the same requirement; the shift amount
+// must be in ecx. In order to handle arbitrary input registers, we divide this
+// operation into phases:
+//
+// [PUSH]     Preserve any registers which may be clobbered
+// [MOVE]     Move the shift to ecx and the amount to be shifted to an
+//            arbitrarily chosen preserved register that is not ecx.
+// [SHIFT]    Do the shift operation
+// [MOVE]     Move the result back to the destination
+// [POP]      Restore the registers which were preserved.
+inline void FlexibleShift32(MacroAssembler& masm, Register shift,
+                            Register srcDest, bool left,
+                            bool arithmetic = false) {
+  // Choose an arbitrary register that's not ecx
+  Register internalSrcDest = (srcDest != ecx) ? srcDest : ebx;
+  MOZ_ASSERT(internalSrcDest != ecx);
+
+  // Add registers we may clobber and want to ensure are restored as live, and
+  // remove what we definitely clobber (the destination)
+  LiveRegisterSet preserve;
+  preserve.add(ecx);
+  preserve.add(internalSrcDest);
+
+  preserve.takeUnchecked(srcDest);
+
+  // [PUSH]
+  masm.PushRegsInMask(preserve);
+
+  // [MOVE]
+  masm.moveRegPair(shift, srcDest, ecx, internalSrcDest);
+  if (masm.oom()) {
+    return;
   }
 
+  // [SHIFT]
   if (left) {
-    masm.lshift32(ecx, dest);
+    masm.lshift32(ecx, internalSrcDest);
   } else {
     if (arithmetic) {
-      masm.rshift32Arithmetic(ecx, dest);
+      masm.rshift32Arithmetic(ecx, internalSrcDest);
     } else {
-      masm.rshift32(ecx, dest);
+      masm.rshift32(ecx, internalSrcDest);
     }
   }
 
-  if (shift != ecx && dest != ecx) {
-    masm.pop(ecx);
+  // [MOVE]
+  if (internalSrcDest != srcDest) {
+    masm.mov(internalSrcDest, srcDest);
   }
+
+  // [POP]
+  masm.PopRegsInMask(preserve);
 }
 
 void MacroAssembler::flexibleLshift32(Register shift, Register srcDest) {
   FlexibleShift32(*this, shift, srcDest, true);
 }
 
 void MacroAssembler::rshift32(Register shift, Register srcDest) {
   MOZ_ASSERT(shift == ecx);
--- a/js/src/jsapi-tests/testJitMacroAssembler.cpp
+++ b/js/src/jsapi-tests/testJitMacroAssembler.cpp
@@ -210,16 +210,216 @@ BEGIN_TEST(testJitMacroAssembler_flexibl
       masm.bind(&next);
     }
   }
 
   return Execute(cx, masm);
 }
 END_TEST(testJitMacroAssembler_flexibleQuotient)
 
+// To make sure ecx isn't being clobbered; globally scoped to ensure it has the
+// right lifetime.
+const uintptr_t guardEcx = 0xfeedbad;
+
+bool shiftTest(JSContext* cx, const char* name,
+               void (*operation)(StackMacroAssembler& masm, Register, Register),
+               const uintptr_t* lhsInput, const uintptr_t* rhsInput,
+               const uintptr_t* result) {
+  StackMacroAssembler masm(cx);
+
+  if (!Prepare(masm)) {
+    return false;
+  }
+
+  JS::AutoSuppressGCAnalysis suppress;
+  AllocatableGeneralRegisterSet leftOutputHandSides(GeneralRegisterSet::All());
+
+  while (!leftOutputHandSides.empty()) {
+    Register lhsOutput = leftOutputHandSides.takeAny();
+
+    AllocatableGeneralRegisterSet rightHandSides(GeneralRegisterSet::All());
+    while (!rightHandSides.empty()) {
+      Register rhs = rightHandSides.takeAny();
+
+      // You can only use shift as the same reg if the values are the same
+      if (lhsOutput == rhs && lhsInput != rhsInput) {
+        continue;
+      }
+
+      Label next, outputFail, clobberRhs, clobberEcx, dump;
+      masm.mov(ImmWord(guardEcx), ecx);
+      masm.mov(ImmWord(*lhsInput), lhsOutput);
+      masm.mov(ImmWord(*rhsInput), rhs);
+
+      operation(masm, rhs, lhsOutput);
+
+      // Ensure Result is correct
+      masm.branch32(Assembler::NotEqual, AbsoluteAddress(result), lhsOutput,
+                    &outputFail);
+
+      // Ensure RHS was not clobbered
+      masm.branch32(Assembler::NotEqual, AbsoluteAddress(rhsInput), rhs,
+                    &clobberRhs);
+
+      if (lhsOutput != ecx && rhs != ecx) {
+        // If neither lhsOutput nor rhs is ecx, make sure ecx has been
+        // preserved, otherwise it's expected to be covered by the RHS clobber
+        // check above, or intentionally clobbered as the output.
+        masm.branch32(Assembler::NotEqual, AbsoluteAddress(&guardEcx), ecx,
+                      &clobberEcx);
+      }
+
+      masm.jump(&next);
+
+      masm.bind(&outputFail);
+      masm.printf("Incorrect output (got %d) ", lhsOutput);
+      masm.jump(&dump);
+
+      masm.bind(&clobberRhs);
+      masm.printf("rhs clobbered %d", rhs);
+      masm.jump(&dump);
+
+      masm.bind(&clobberEcx);
+      masm.printf("ecx clobbered");
+      masm.jump(&dump);
+
+      masm.bind(&dump);
+      masm.mov(ImmPtr(lhsOutput.name()), lhsOutput);
+      masm.printf("(lhsOutput/srcDest) %s ", lhsOutput);
+      masm.mov(ImmPtr(name), lhsOutput);
+      masm.printf("%s ", lhsOutput);
+      masm.mov(ImmPtr(rhs.name()), lhsOutput);
+      masm.printf("(shift/rhs) %s \n", lhsOutput);
+      // Breakpoint to force test failure.
+      masm.breakpoint();
+      masm.bind(&next);
+    }
+  }
+
+  return Execute(cx, masm);
+}
+
+BEGIN_TEST(testJitMacroAssembler_flexibleRshift) {
+  {
+    // Test case  16 >> 2 == 4;
+    const uintptr_t lhsInput = 16;
+    const uintptr_t rhsInput = 2;
+    const uintptr_t result = 4;
+
+    bool res = shiftTest(
+        cx, "flexibleRshift32",
+        [](StackMacroAssembler& masm, Register rhs, Register lhsOutput) {
+          masm.flexibleRshift32(rhs, lhsOutput);
+        },
+        &lhsInput, &rhsInput, &result);
+    if (!res) {
+      return false;
+    }
+  }
+
+  {
+    // Test case  16 >> 16 == 0 -- this helps cover the case where the same
+    // register can be passed for source and dest.
+    const uintptr_t lhsInput = 16;
+    const uintptr_t rhsInput = 16;
+    const uintptr_t result = 0;
+
+    bool res = shiftTest(
+        cx, "flexibleRshift32",
+        [](StackMacroAssembler& masm, Register rhs, Register lhsOutput) {
+          masm.flexibleRshift32(rhs, lhsOutput);
+        },
+        &lhsInput, &rhsInput, &result);
+    if (!res) {
+      return false;
+    }
+  }
+
+  return true;
+}
+END_TEST(testJitMacroAssembler_flexibleRshift)
+
+BEGIN_TEST(testJitMacroAssembler_flexibleRshiftArithmetic) {
+  {
+    // Test case  4294967295 >> 2 == 4294967295;
+    const uintptr_t lhsInput = 4294967295;
+    const uintptr_t rhsInput = 2;
+    const uintptr_t result = 4294967295;
+    bool res = shiftTest(
+        cx, "flexibleRshift32Arithmetic",
+        [](StackMacroAssembler& masm, Register rhs, Register lhsOutput) {
+          masm.flexibleRshift32Arithmetic(rhs, lhsOutput);
+        },
+        &lhsInput, &rhsInput, &result);
+    if (!res) {
+      return false;
+    }
+  }
+
+  {
+    // Test case  16 >> 16 == 0 -- this helps cover the case where the same
+    // register can be passed for source and dest.
+    const uintptr_t lhsInput = 16;
+    const uintptr_t rhsInput = 16;
+    const uintptr_t result = 0;
+
+    bool res = shiftTest(
+        cx, "flexibleRshift32Arithmetic",
+        [](StackMacroAssembler& masm, Register rhs, Register lhsOutput) {
+          masm.flexibleRshift32Arithmetic(rhs, lhsOutput);
+        },
+        &lhsInput, &rhsInput, &result);
+    if (!res) {
+      return false;
+    }
+  }
+
+  return true;
+}
+END_TEST(testJitMacroAssembler_flexibleRshiftArithmetic)
+
+BEGIN_TEST(testJitMacroAssembler_flexibleLshift) {
+  {
+    // Test case  16 << 2 == 64;
+    const uintptr_t lhsInput = 16;
+    const uintptr_t rhsInput = 2;
+    const uintptr_t result = 64;
+
+    bool res = shiftTest(
+        cx, "flexibleLshift32",
+        [](StackMacroAssembler& masm, Register rhs, Register lhsOutput) {
+          masm.flexibleLshift32(rhs, lhsOutput);
+        },
+        &lhsInput, &rhsInput, &result);
+    if (!res) {
+      return false;
+    }
+  }
+
+  {
+    // Test case  4 << 4 == 64; duplicated input case
+    const uintptr_t lhsInput = 4;
+    const uintptr_t rhsInput = 4;
+    const uintptr_t result = 64;
+
+    bool res = shiftTest(
+        cx, "flexibleLshift32",
+        [](StackMacroAssembler& masm, Register rhs, Register lhsOutput) {
+          masm.flexibleLshift32(rhs, lhsOutput);
+        },
+        &lhsInput, &rhsInput, &result);
+    if (!res) {
+      return false;
+    }
+  }
+
+  return true;
+}
+END_TEST(testJitMacroAssembler_flexibleLshift)
+
 BEGIN_TEST(testJitMacroAssembler_truncateDoubleToInt64) {
   StackMacroAssembler masm(cx);
 
   if (!Prepare(masm)) {
     return false;
   }
 
   AllocatableGeneralRegisterSet allRegs(GeneralRegisterSet::All());