Bug 1538692 - Part 1: Support relational string comparison in Ion. r=mgaudet
authorAndré Bargull <andre.bargull@gmail.com>
Tue, 26 Mar 2019 14:54:34 +0000
changeset 466200 998ea689fe149c52983e32af9e7cf50a8cc95c37
parent 466199 20081e9a97193748d9dc96040afcbf34d7dbca5a
child 466201 e30a80c7854ff12c5c792a10402eed2d4d30c56e
push id35762
push usercsabou@mozilla.com
push dateWed, 27 Mar 2019 04:44:00 +0000
treeherdermozilla-central@bc572aee49b6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmgaudet
bugs1538692
milestone68.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 1538692 - Part 1: Support relational string comparison in Ion. r=mgaudet Add jit::StringsCompare to call js::CompareStrings, mirroring the existing jit::StringsEqual and js::EqualStrings pair for equality comparison. JSOP_LE and JSOP_GT are implemented by pushing the operands in reverse order and then calling jit::StringsCompare for JSOP_LT resp. JSOP_GE. This avoids creating four different VMFunction wrappers and also matches how the ECMAScript spec defines relational comparison evaluation. ion/compare-string.js - Add relational comparison operators. - Ensure string rope tests are actually using ropes. - Lower iteration count to reduce time needed to complete test for --tbpl configuration. Differential Revision: https://phabricator.services.mozilla.com/D24706
js/src/jit-test/tests/ion/compare-string.js
js/src/jit/CacheIR.cpp
js/src/jit/CodeGenerator.cpp
js/src/jit/IonAnalysis.cpp
js/src/jit/MIR.cpp
js/src/jit/MacroAssembler.cpp
js/src/jit/VMFunctionList-inl.h
js/src/jit/VMFunctions.cpp
js/src/jit/VMFunctions.h
js/src/vm/BytecodeUtil.h
--- a/js/src/jit-test/tests/ion/compare-string.js
+++ b/js/src/jit-test/tests/ion/compare-string.js
@@ -1,40 +1,131 @@
 function compareToAtom(a) {
-    return a == 'test';
+    return a == 'test-test-test-test-test-test-test-test';
+}
+
+function compareToAtomStrict(a) {
+    return a === 'test-test-test-test-test-test-test-test';
 }
 
 function compareToAtomNe(a) {
-    return a != 'test';
+    return a != 'test-test-test-test-test-test-test-test';
+}
+
+function compareToAtomNeStrict(a) {
+    return a !== 'test-test-test-test-test-test-test-test';
+}
+
+function compareToAtomLessThan(a) {
+    return a < 'test-test-test-test-test-test-test-test';
 }
 
-var st = 'st';
+function compareToAtomLessThanOrEquals(a) {
+    return a <= 'test-test-test-test-test-test-test-test';
+}
+
+function compareToAtomGreaterThan(a) {
+    return a > 'test-test-test-test-test-test-test-test';
+}
+
+function compareToAtomGreaterThanOrEquals(a) {
+    return a >= 'test-test-test-test-test-test-test-test';
+}
+
+var st = 'st-test-test-test-test-test-test-test';
 
 function compareToRope(a) {
     return a == ('te' + st);
 }
 
+function compareToRopeStrict(a) {
+    return a === ('te' + st);
+}
+
 function compareToRopeNe(a) {
-    var st = 'st';
+    var st = 'st-test-test-test-test-test-test-test';
     return a != ('te' + st);
 }
 
+function compareToRopeNeStrict(a) {
+    var st = 'st-test-test-test-test-test-test-test';
+    return a !== ('te' + st);
+}
+
+function compareToRopeLessThan(a) {
+    var st = 'st-test-test-test-test-test-test-test';
+    return a < ('te' + st);
+}
+
+function compareToRopeLessThanOrEquals(a) {
+    var st = 'st-test-test-test-test-test-test-test';
+    return a <= ('te' + st);
+}
+
+function compareToRopeGreaterThan(a) {
+    var st = 'st-test-test-test-test-test-test-test';
+    return a > ('te' + st);
+}
+
+function compareToRopeGreaterThanOrEquals(a) {
+    var st = 'st-test-test-test-test-test-test-test';
+    return a >= ('te' + st);
+}
+
 function main() {
-    var test = 'test';
+    // |test| must be longer than |JSFatInlineString::MAX_LENGTH_LATIN1| to
+    // ensure the above functions create ropes when concatenating strings.
+    var test = 'test-test-test-test-test-test-test-test';
     var foobar = 'foobar';
 
     assertEq(compareToAtom(test), true);
     assertEq(compareToAtom(foobar), false);
 
+    assertEq(compareToAtomStrict(test), true);
+    assertEq(compareToAtomStrict(foobar), false);
+
     assertEq(compareToAtomNe(test), false);
     assertEq(compareToAtomNe(foobar), true);
 
+    assertEq(compareToAtomNeStrict(test), false);
+    assertEq(compareToAtomNeStrict(foobar), true);
+
+    assertEq(compareToAtomLessThan(test), false);
+    assertEq(compareToAtomLessThan(foobar), true);
+
+    assertEq(compareToAtomLessThanOrEquals(test), true);
+    assertEq(compareToAtomLessThanOrEquals(foobar), true);
+
+    assertEq(compareToAtomGreaterThan(test), false);
+    assertEq(compareToAtomGreaterThan(foobar), false);
+
+    assertEq(compareToAtomGreaterThanOrEquals(test), true);
+    assertEq(compareToAtomGreaterThanOrEquals(foobar), false);
+
 
     assertEq(compareToRope(test), true);
     assertEq(compareToRope(foobar), false);
 
+    assertEq(compareToRopeStrict(test), true);
+    assertEq(compareToRopeStrict(foobar), false);
+
     assertEq(compareToRopeNe(test), false);
     assertEq(compareToRopeNe(foobar), true);
+
+    assertEq(compareToRopeNeStrict(test), false);
+    assertEq(compareToRopeNeStrict(foobar), true);
+
+    assertEq(compareToRopeLessThan(test), false);
+    assertEq(compareToRopeLessThan(foobar), true);
+
+    assertEq(compareToRopeLessThanOrEquals(test), true);
+    assertEq(compareToRopeLessThanOrEquals(foobar), true);
+
+    assertEq(compareToRopeGreaterThan(test), false);
+    assertEq(compareToRopeGreaterThan(foobar), false);
+
+    assertEq(compareToRopeGreaterThanOrEquals(test), true);
+    assertEq(compareToRopeGreaterThanOrEquals(foobar), false);
 }
 
-for (var i = 0; i < 100000; i++) {
+for (var i = 0; i < 10000; i++) {
     main();
 }
--- a/js/src/jit/CacheIR.cpp
+++ b/js/src/jit/CacheIR.cpp
@@ -5737,18 +5737,17 @@ bool CompareIRGenerator::tryAttachString
   writer.returnFromIC();
 
   trackAttached("StringNumber");
   return true;
 }
 
 bool CompareIRGenerator::tryAttachStub() {
   MOZ_ASSERT(cacheKind_ == CacheKind::Compare);
-  MOZ_ASSERT(IsEqualityOp(op_) || op_ == JSOP_LE || op_ == JSOP_LT ||
-             op_ == JSOP_GE || op_ == JSOP_GT);
+  MOZ_ASSERT(IsEqualityOp(op_) || IsRelationalOp(op_));
 
   AutoAssertNoPendingException aanpe(cx_);
 
   constexpr uint8_t lhsIndex = 0;
   constexpr uint8_t rhsIndex = 1;
 
   static_assert(lhsIndex == 0 && rhsIndex == 1,
                 "Indexes relied upon by baseline inspector");
--- a/js/src/jit/CodeGenerator.cpp
+++ b/js/src/jit/CodeGenerator.cpp
@@ -8040,20 +8040,36 @@ void CodeGenerator::emitCompareS(LInstru
   MOZ_ASSERT(lir->isCompareS() || lir->isCompareStrictS());
 
   OutOfLineCode* ool = nullptr;
 
   using Fn = bool (*)(JSContext*, HandleString, HandleString, bool*);
   if (op == JSOP_EQ || op == JSOP_STRICTEQ) {
     ool = oolCallVM<Fn, jit::StringsEqual<true>>(lir, ArgList(left, right),
                                                  StoreRegisterTo(output));
-  } else {
-    MOZ_ASSERT(op == JSOP_NE || op == JSOP_STRICTNE);
+  } else if (op == JSOP_NE || op == JSOP_STRICTNE) {
     ool = oolCallVM<Fn, jit::StringsEqual<false>>(lir, ArgList(left, right),
                                                   StoreRegisterTo(output));
+  } else if (op == JSOP_LT) {
+    ool = oolCallVM<Fn, jit::StringsCompare<true>>(lir, ArgList(left, right),
+                                                   StoreRegisterTo(output));
+  } else if (op == JSOP_LE) {
+    // Push the operands in reverse order for JSOP_LE:
+    // - |left <= right| is implemented as |right >= left|.
+    ool = oolCallVM<Fn, jit::StringsCompare<false>>(lir, ArgList(right, left),
+                                                    StoreRegisterTo(output));
+  } else if (op == JSOP_GT) {
+    // Push the operands in reverse order for JSOP_GT:
+    // - |left > right| is implemented as |right < left|.
+    ool = oolCallVM<Fn, jit::StringsCompare<true>>(lir, ArgList(right, left),
+                                                   StoreRegisterTo(output));
+  } else {
+    MOZ_ASSERT(op == JSOP_GE);
+    ool = oolCallVM<Fn, jit::StringsCompare<false>>(lir, ArgList(left, right),
+                                                    StoreRegisterTo(output));
   }
 
   masm.compareStrings(op, left, right, output, ool->entry());
 
   masm.bind(ool->rejoin());
 }
 
 void CodeGenerator::visitCompareStrictS(LCompareStrictS* lir) {
--- a/js/src/jit/IonAnalysis.cpp
+++ b/js/src/jit/IonAnalysis.cpp
@@ -2356,18 +2356,17 @@ static bool CanCompareRegExp(MCompare* c
   JSOp op = compare->jsop();
   // Strict equality comparison won't invoke @@toPrimitive.
   if (op == JSOP_STRICTEQ || op == JSOP_STRICTNE) {
     return true;
   }
 
   if (op != JSOP_EQ && op != JSOP_NE) {
     // Relational comparison always invoke @@toPrimitive.
-    MOZ_ASSERT(op == JSOP_GT || op == JSOP_GE || op == JSOP_LT ||
-               op == JSOP_LE);
+    MOZ_ASSERT(IsRelationalOp(op));
     return false;
   }
 
   // Loose equality comparison can invoke @@toPrimitive.
   if (value->mightBeType(MIRType::Boolean) ||
       value->mightBeType(MIRType::String) ||
       value->mightBeType(MIRType::Int32) ||
       value->mightBeType(MIRType::Double) ||
--- a/js/src/jit/MIR.cpp
+++ b/js/src/jit/MIR.cpp
@@ -3468,19 +3468,18 @@ MCompare::CompareType MCompare::determin
     return Compare_DoubleMaybeCoerceRHS;
   }
 
   // Handle object comparison.
   if (!relationalEq && lhs == MIRType::Object && rhs == MIRType::Object) {
     return Compare_Object;
   }
 
-  // Handle string comparisons. (Relational string compares are still
-  // unsupported).
-  if (!relationalEq && lhs == MIRType::String && rhs == MIRType::String) {
+  // Handle string comparisons.
+  if (lhs == MIRType::String && rhs == MIRType::String) {
     return Compare_String;
   }
 
   // Handle symbol comparisons. (Relational compare will throw)
   if (!relationalEq && lhs == MIRType::Symbol && rhs == MIRType::Symbol) {
     return Compare_Symbol;
   }
 
--- a/js/src/jit/MacroAssembler.cpp
+++ b/js/src/jit/MacroAssembler.cpp
@@ -1201,46 +1201,52 @@ void MacroAssembler::initGCThing(Registe
 #endif
 }
 
 
 void MacroAssembler::compareStrings(JSOp op, Register left, Register right,
                                     Register result, Label* fail) {
   MOZ_ASSERT(left != result);
   MOZ_ASSERT(right != result);
-  MOZ_ASSERT(IsEqualityOp(op));
-
-  Label done;
+  MOZ_ASSERT(IsEqualityOp(op) || IsRelationalOp(op));
+
   Label notPointerEqual;
   // If operands point to the same instance, the strings are trivially equal.
-  branchPtr(Assembler::NotEqual, left, right, &notPointerEqual);
-  move32(Imm32(op == JSOP_EQ || op == JSOP_STRICTEQ), result);
-  jump(&done);
-
-  bind(&notPointerEqual);
-
-  Label leftIsNotAtom;
-  Label setNotEqualResult;
-  // Atoms cannot be equal to each other if they point to different strings.
-  Imm32 nonAtomBit(JSString::NON_ATOM_BIT);
-  branchTest32(Assembler::NonZero, Address(left, JSString::offsetOfFlags()),
-               nonAtomBit, &leftIsNotAtom);
-  branchTest32(Assembler::Zero, Address(right, JSString::offsetOfFlags()),
-               nonAtomBit, &setNotEqualResult);
-
-  bind(&leftIsNotAtom);
-  // Strings of different length can never be equal.
-  loadStringLength(left, result);
-  branch32(Assembler::Equal, Address(right, JSString::offsetOfLength()), result,
-           fail);
-
-  bind(&setNotEqualResult);
-  move32(Imm32(op == JSOP_NE || op == JSOP_STRICTNE), result);
-
-  bind(&done);
+  branchPtr(Assembler::NotEqual, left, right,
+            IsEqualityOp(op) ? &notPointerEqual : fail);
+  move32(Imm32(op == JSOP_EQ || op == JSOP_STRICTEQ || op == JSOP_LE ||
+               op == JSOP_GE),
+         result);
+
+  if (IsEqualityOp(op)) {
+    Label done;
+    jump(&done);
+
+    bind(&notPointerEqual);
+
+    Label leftIsNotAtom;
+    Label setNotEqualResult;
+    // Atoms cannot be equal to each other if they point to different strings.
+    Imm32 nonAtomBit(JSString::NON_ATOM_BIT);
+    branchTest32(Assembler::NonZero, Address(left, JSString::offsetOfFlags()),
+                 nonAtomBit, &leftIsNotAtom);
+    branchTest32(Assembler::Zero, Address(right, JSString::offsetOfFlags()),
+                 nonAtomBit, &setNotEqualResult);
+
+    bind(&leftIsNotAtom);
+    // Strings of different length can never be equal.
+    loadStringLength(left, result);
+    branch32(Assembler::Equal, Address(right, JSString::offsetOfLength()),
+             result, fail);
+
+    bind(&setNotEqualResult);
+    move32(Imm32(op == JSOP_NE || op == JSOP_STRICTNE), result);
+
+    bind(&done);
+  }
 }
 
 void MacroAssembler::loadStringChars(Register str, Register dest,
                                      CharEncoding encoding) {
   MOZ_ASSERT(str != dest);
 
   if (JitOptions.spectreStringMitigations) {
     if (encoding == CharEncoding::Latin1) {
--- a/js/src/jit/VMFunctionList-inl.h
+++ b/js/src/jit/VMFunctionList-inl.h
@@ -224,16 +224,18 @@ namespace jit {
   _(StringFromCharCode, js::jit::StringFromCharCode)                           \
   _(StringFromCodePoint, js::jit::StringFromCodePoint)                         \
   _(StringReplace, js::jit::StringReplace)                                     \
   _(StringSplitHelper, js::jit::StringSplitHelper)                             \
   _(StringSplitString, js::StringSplitString)                                  \
   _(StringToLowerCase, js::StringToLowerCase)                                  \
   _(StringToNumber, js::StringToNumber)                                        \
   _(StringToUpperCase, js::StringToUpperCase)                                  \
+  _(StringsCompareGreaterThanOrEquals, js::jit::StringsCompare<false>)         \
+  _(StringsCompareLessThan, js::jit::StringsCompare<true>)                     \
   _(StringsEqual, js::jit::StringsEqual<true>)                                 \
   _(StringsNotEqual, js::jit::StringsEqual<false>)                             \
   _(SubValues, js::SubValues)                                                  \
   _(SubstringKernel, js::SubstringKernel)                                      \
   _(SuperFunOperation, js::SuperFunOperation)                                  \
   _(ThrowBadDerivedReturn, js::jit::ThrowBadDerivedReturn)                     \
   _(ThrowCheckIsObject, js::ThrowCheckIsObject)                                \
   _(ThrowMsgOperation, js::ThrowMsgOperation)                                  \
--- a/js/src/jit/VMFunctions.cpp
+++ b/js/src/jit/VMFunctions.cpp
@@ -409,16 +409,36 @@ bool StringsEqual(JSContext* cx, HandleS
   return true;
 }
 
 template bool StringsEqual<true>(JSContext* cx, HandleString lhs,
                                  HandleString rhs, bool* res);
 template bool StringsEqual<false>(JSContext* cx, HandleString lhs,
                                   HandleString rhs, bool* res);
 
+template <bool LessThan>
+bool StringsCompare(JSContext* cx, HandleString lhs, HandleString rhs,
+                    bool* res) {
+  int32_t result;
+  if (!js::CompareStrings(cx, lhs, rhs, &result)) {
+    return false;
+  }
+  if (LessThan) {
+    *res = result < 0;
+  } else {
+    *res = result >= 0;
+  }
+  return true;
+}
+
+template bool StringsCompare<true>(JSContext* cx, HandleString lhs,
+                                   HandleString rhs, bool* res);
+template bool StringsCompare<false>(JSContext* cx, HandleString lhs,
+                                    HandleString rhs, bool* res);
+
 bool StringSplitHelper(JSContext* cx, HandleString str, HandleString sep,
                        HandleObjectGroup group, uint32_t limit,
                        MutableHandleValue result) {
   JSObject* resultObj = StringSplitString(cx, group, str, sep, limit);
   if (!resultObj) {
     return false;
   }
 
--- a/js/src/jit/VMFunctions.h
+++ b/js/src/jit/VMFunctions.h
@@ -851,18 +851,21 @@ bool LessThan(JSContext* cx, MutableHand
 bool LessThanOrEqual(JSContext* cx, MutableHandleValue lhs,
                      MutableHandleValue rhs, bool* res);
 bool GreaterThan(JSContext* cx, MutableHandleValue lhs, MutableHandleValue rhs,
                  bool* res);
 bool GreaterThanOrEqual(JSContext* cx, MutableHandleValue lhs,
                         MutableHandleValue rhs, bool* res);
 
 template <bool Equal>
-bool StringsEqual(JSContext* cx, HandleString left, HandleString right,
-                  bool* res);
+bool StringsEqual(JSContext* cx, HandleString lhs, HandleString rhs, bool* res);
+
+template <bool LessThan>
+bool StringsCompare(JSContext* cx, HandleString lhs, HandleString rhs,
+                    bool* res);
 
 MOZ_MUST_USE bool StringSplitHelper(JSContext* cx, HandleString str,
                                     HandleString sep, HandleObjectGroup group,
                                     uint32_t limit, MutableHandleValue result);
 
 MOZ_MUST_USE bool ArrayPopDense(JSContext* cx, HandleObject obj,
                                 MutableHandleValue rval);
 MOZ_MUST_USE bool ArrayPushDense(JSContext* cx, HandleArrayObject arr,
--- a/js/src/vm/BytecodeUtil.h
+++ b/js/src/vm/BytecodeUtil.h
@@ -530,16 +530,20 @@ inline bool IsPropertyInitOp(JSOp op) {
   return CodeSpec[op].format & JOF_PROPINIT;
 }
 
 inline bool IsEqualityOp(JSOp op) {
   return op == JSOP_EQ || op == JSOP_NE || op == JSOP_STRICTEQ ||
          op == JSOP_STRICTNE;
 }
 
+inline bool IsRelationalOp(JSOp op) {
+  return op == JSOP_LT || op == JSOP_LE || op == JSOP_GT || op == JSOP_GE;
+}
+
 inline bool IsCheckStrictOp(JSOp op) {
   return CodeSpec[op].format & JOF_CHECKSTRICT;
 }
 
 #ifdef DEBUG
 inline bool IsCheckSloppyOp(JSOp op) {
   return CodeSpec[op].format & JOF_CHECKSLOPPY;
 }