Bug 1302367 - Regression in Emscripten's conditionals caused by ModD r?nbp,jandem draft
authorSander Mathijs van Veen <sander@leaningtech.com>
Tue, 25 Oct 2016 13:04:59 +0000
changeset 429198 7d1b7bb9ce604ad267f879fe4db00b396b38d324
parent 425712 9079d167112122805f99f57bb8856e1b1675af0f
child 534921 0bc993e699e798182ca5d20350f00bec955bc31f
push id33509
push usersandervv@gmail.com
push dateTue, 25 Oct 2016 13:11:52 +0000
reviewersnbp, jandem
bugs1302367
milestone52.0a1
Bug 1302367 - Regression in Emscripten's conditionals caused by ModD r?nbp,jandem MozReview-Commit-ID: DotMfdJ8zBD
js/src/jit/IonAnalysis.cpp
js/src/jit/IonAnalysis.h
js/src/jit/MIR.cpp
js/src/jit/MIR.h
js/src/jit/RangeAnalysis.cpp
--- a/js/src/jit/IonAnalysis.cpp
+++ b/js/src/jit/IonAnalysis.cpp
@@ -564,16 +564,30 @@ jit::SplitCriticalEdges(MIRGraph& graph)
     for (MBasicBlockIterator iter(graph.begin()); iter != graph.end(); iter++) {
         MBasicBlock* block = *iter;
         if (!SplitCriticalEdgesForBlock(graph, block))
             return false;
     }
     return true;
 }
 
+bool
+jit::IsUint32Type(const MDefinition* def)
+{
+    if (def->isBeta())
+        def = def->getOperand(0);
+
+    if (def->type() != MIRType::Int32)
+        return false;
+
+    return def->isUrsh() && def->getOperand(1)->isConstant() &&
+        def->getOperand(1)->toConstant()->type() == MIRType::Int32 &&
+        def->getOperand(1)->toConstant()->toInt32() == 0;
+}
+
 // Return whether a block simply computes the specified constant value.
 static bool
 BlockComputesConstant(MBasicBlock* block, MDefinition* value, bool* constBool)
 {
     // Look for values with no uses. This is used to eliminate constant
     // computing blocks in condition statements, and the phi which used to
     // consume the constant has already been removed.
     if (value->hasUses())
--- a/js/src/jit/IonAnalysis.h
+++ b/js/src/jit/IonAnalysis.h
@@ -22,16 +22,19 @@ MOZ_MUST_USE bool
 PruneUnusedBranches(MIRGenerator* mir, MIRGraph& graph);
 
 MOZ_MUST_USE bool
 FoldTests(MIRGraph& graph);
 
 MOZ_MUST_USE bool
 SplitCriticalEdges(MIRGraph& graph);
 
+bool
+IsUint32Type(const MDefinition* def);
+
 enum Observability {
     ConservativeObservability,
     AggressiveObservability
 };
 
 MOZ_MUST_USE bool
 EliminatePhis(MIRGenerator* mir, MIRGraph& graph, Observability observe);
 
--- a/js/src/jit/MIR.cpp
+++ b/js/src/jit/MIR.cpp
@@ -2734,16 +2734,27 @@ MBinaryBitwiseInstruction::foldsTo(TempA
 }
 
 MDefinition*
 MBinaryBitwiseInstruction::foldUnnecessaryBitop()
 {
     if (specialization_ != MIRType::Int32)
         return this;
 
+    // Fold unsigned shift right operator when the second operand is zero and
+    // the only use is an unsigned modulo. Thus, the expression
+    // |(x >>> 0) % y| becomes |x % y|.
+    if (isUrsh() && hasOneDefUse() && IsUint32Type(this)) {
+        for (MUseDefIterator use(this); use; use++) {
+            if (use.def()->isMod() && use.def()->toMod()->isUnsigned())
+                return getOperand(0);
+            break;
+        }
+    }
+
     // Eliminate bitwise operations that are no-ops when used on integer
     // inputs, such as (x | 0).
 
     MDefinition* lhs = getOperand(0);
     MDefinition* rhs = getOperand(1);
 
     if (IsConstant(lhs, 0))
         return foldIfZero(0);
@@ -2955,16 +2966,49 @@ NeedNegativeZeroCheck(MDefinition* def)
             break;
           default:
             return true;
         }
     }
     return false;
 }
 
+void
+MBinaryArithInstruction::printOpcode(GenericPrinter& out) const
+{
+    MDefinition::printOpcode(out);
+
+    switch (type()) {
+      case MIRType::Int32:
+        if (isDiv())
+            out.printf(" [%s]", toDiv()->isUnsigned() ? "uint32" : "int32");
+        else if (isMod())
+            out.printf(" [%s]", toMod()->isUnsigned() ? "uint32" : "int32");
+        else
+            out.printf(" [int32]");
+        break;
+      case MIRType::Int64:
+        if (isDiv())
+            out.printf(" [%s]", toDiv()->isUnsigned() ? "uint64" : "int64");
+        else if (isMod())
+            out.printf(" [%s]", toMod()->isUnsigned() ? "uint64" : "int64");
+        else
+            out.printf(" [int64]");
+        break;
+      case MIRType::Float32:
+        out.printf(" [float]");
+        break;
+      case MIRType::Double:
+        out.printf(" [double]");
+        break;
+      default:
+        break;
+    }
+}
+
 MBinaryArithInstruction*
 MBinaryArithInstruction::New(TempAllocator& alloc, Opcode op,
                              MDefinition* left, MDefinition* right)
 {
     switch (op) {
       case Op_Add:
         return MAdd::New(alloc, left, right);
       case Op_Sub:
@@ -4070,18 +4114,27 @@ MToInt32::foldsTo(TempAllocator& alloc)
             if (mozilla::NumberEqualsInt32(input->toConstant()->numberToDouble(), &ival))
                 return MConstant::New(alloc, Int32Value(ival));
             break;
           default:
             break;
         }
     }
 
-    if (input->type() == MIRType::Int32)
+    // Do not fold the TruncateToInt32 node when the input is uint32 (e.g. ursh
+    // with a zero constant. Consider the test jit-test/tests/ion/bug1247880.js,
+    // where the relevant code is: |(imul(1, x >>> 0) % 2)|. The imul operator
+    // is folded to a MTruncateToInt32 node, which will result in this MIR:
+    // MMod(MTruncateToInt32(MUrsh(x, MConstant(0))), MConstant(2)). Note that
+    // the MUrsh node's type is int32 (since uint32 is not implemented), and
+    // that would fold the MTruncateToInt32 node. This will make the modulo
+    // unsigned, while is should have been signed.
+    if (input->type() == MIRType::Int32 && !IsUint32Type(input))
         return input;
+
     return this;
 }
 
 void
 MToInt32::analyzeEdgeCasesBackward()
 {
     if (!NeedNegativeZeroCheck(this))
         setCanBeNegativeZero(false);
@@ -4089,17 +4142,25 @@ MToInt32::analyzeEdgeCasesBackward()
 
 MDefinition*
 MTruncateToInt32::foldsTo(TempAllocator& alloc)
 {
     MDefinition* input = getOperand(0);
     if (input->isBox())
         input = input->getOperand(0);
 
-    if (input->type() == MIRType::Int32)
+    // Do not fold the TruncateToInt32 node when the input is uint32 (e.g. ursh
+    // with a zero constant. Consider the test jit-test/tests/ion/bug1247880.js,
+    // where the relevant code is: |(imul(1, x >>> 0) % 2)|. The imul operator
+    // is folded to a MTruncateToInt32 node, which will result in this MIR:
+    // MMod(MTruncateToInt32(MUrsh(x, MConstant(0))), MConstant(2)). Note that
+    // the MUrsh node's type is int32 (since uint32 is not implemented), and
+    // that would fold the MTruncateToInt32 node. This will make the modulo
+    // unsigned, while is should have been signed.
+    if (input->type() == MIRType::Int32 && !IsUint32Type(input))
         return input;
 
     if (input->type() == MIRType::Double && input->isConstant()) {
         int32_t ret = ToInt32(input->toConstant()->toDouble());
         return MConstant::New(alloc, Int32Value(ret));
     }
 
     return this;
--- a/js/src/jit/MIR.h
+++ b/js/src/jit/MIR.h
@@ -6178,16 +6178,17 @@ class MBinaryArithInstruction
                                         MDefinition* left, MDefinition* right);
 
     bool constantDoubleResult(TempAllocator& alloc);
 
     void setMustPreserveNaN(bool b) { mustPreserveNaN_ = b; }
     bool mustPreserveNaN() const { return mustPreserveNaN_; }
 
     MDefinition* foldsTo(TempAllocator& alloc) override;
+    void printOpcode(GenericPrinter& out) const override;
 
     virtual double getIdentity() = 0;
 
     void setSpecialization(MIRType type) {
         specialization_ = type;
         setResultType(type);
     }
     void setInt32Specialization() {
--- a/js/src/jit/RangeAnalysis.cpp
+++ b/js/src/jit/RangeAnalysis.cpp
@@ -1565,20 +1565,28 @@ MMod::computeRange(TempAllocator& alloc)
         return;
 
     // If RHS can be zero, the result can be NaN.
     if (rhs.lower() <= 0 && rhs.upper() >= 0)
         return;
 
     // If both operands are non-negative integers, we can optimize this to an
     // unsigned mod.
-    if (specialization() == MIRType::Int32 && lhs.lower() >= 0 && rhs.lower() > 0 &&
-        !lhs.canHaveFractionalPart() && !rhs.canHaveFractionalPart())
-    {
-        unsigned_ = true;
+    if (specialization() == MIRType::Int32 && rhs.lower() > 0) {
+        bool hasDoubles = lhs.lower() < 0 || lhs.canHaveFractionalPart() ||
+            rhs.canHaveFractionalPart();
+        // It is not possible to check that lhs.lower() >= 0, since the range
+        // of a ursh with rhs a 0 constant is wrapped around the int32 range in
+        // Range::Range(). However, IsUint32Type() will only return true for
+        // nodes that lie in the range [0, UINT32_MAX].
+        bool hasUint32s = IsUint32Type(getOperand(0)) &&
+            getOperand(1)->type() == MIRType::Int32 &&
+            (IsUint32Type(getOperand(1)) || getOperand(1)->isConstant());
+        if (!hasDoubles || hasUint32s)
+            unsigned_ = true;
     }
 
     // For unsigned mod, we have to convert both operands to unsigned.
     // Note that we handled the case of a zero rhs above.
     if (unsigned_) {
         // The result of an unsigned mod will never be unsigned-greater than
         // either operand.
         uint32_t lhsBound = Max<uint32_t>(lhs.lower(), lhs.upper());