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

--HG--
extra : moz-landing-system : lando

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';
 }
 
-var st = 'st';
+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';
+}
+
+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();
 }

js/src/jit/CacheIR.cpp

@@ -5742,8 +5742,7 @@ bool CompareIRGenerator::tryAttachStringNumber(ValOperandId lhsId,
 
 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_);
 

js/src/jit/CodeGenerator.cpp

@@ -8045,10 +8045,26 @@ void CodeGenerator::emitCompareS(LInstruction* lir, JSOp op, Register left,
   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());

js/src/jit/IonAnalysis.cpp

@@ -2361,8 +2361,7 @@ static bool CanCompareRegExp(MCompare* compare, MDefinition* def) {
 
   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;
   }
 

js/src/jit/MIR.cpp

@@ -3473,9 +3473,8 @@ MCompare::CompareType MCompare::determineCompareType(JSOp op, MDefinition* left,
     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;
   }
 

js/src/jit/MacroAssembler.cpp

@@ -1206,36 +1206,42 @@ 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));
+  MOZ_ASSERT(IsEqualityOp(op) || IsRelationalOp(op));
 
-  Label done;
   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);
+  branchPtr(Assembler::NotEqual, left, right,
+            IsEqualityOp(op) ? &notPointerEqual : fail);
+  move32(Imm32(op == JSOP_EQ || op == JSOP_STRICTEQ || op == JSOP_LE ||
+               op == JSOP_GE),
+         result);
 
-  bind(&notPointerEqual);
+  if (IsEqualityOp(op)) {
+    Label done;
+    jump(&done);
 
-  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(&notPointerEqual);
 
-  bind(&leftIsNotAtom);
-  // Strings of different length can never be equal.
-  loadStringLength(left, result);
-  branch32(Assembler::Equal, Address(right, JSString::offsetOfLength()), result,
-           fail);
+    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(&setNotEqualResult);
-  move32(Imm32(op == JSOP_NE || op == JSOP_STRICTNE), result);
+    bind(&leftIsNotAtom);
+    // Strings of different length can never be equal.
+    loadStringLength(left, result);
+    branch32(Assembler::Equal, Address(right, JSString::offsetOfLength()),
+             result, fail);
 
-  bind(&done);
+    bind(&setNotEqualResult);
+    move32(Imm32(op == JSOP_NE || op == JSOP_STRICTNE), result);
+
+    bind(&done);
+  }
 }
 
 void MacroAssembler::loadStringChars(Register str, Register dest,

js/src/jit/VMFunctionList-inl.h

@@ -229,6 +229,8 @@ namespace jit {
   _(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)                                                  \

js/src/jit/VMFunctions.cpp

@@ -414,6 +414,26 @@ template bool StringsEqual<true>(JSContext* cx, HandleString lhs,
 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) {

js/src/jit/VMFunctions.h

@@ -856,8 +856,11 @@ 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,

js/src/vm/BytecodeUtil.h

@@ -535,6 +535,10 @@ inline bool IsEqualityOp(JSOp op) {
          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;
 }