Changeset 229513 in webkit for trunk/Source/JavaScriptCore/b3/B3ReduceStrength.cpp

Timestamp:

Mar 10, 2018, 11:16:15 PM (7 years ago)

Author:

Yusuke Suzuki

Message:

B3::reduceStrength should canonicalize integer comparisons
​https://bugs.webkit.org/show_bug.cgi?id=150958

Reviewed by Filip Pizlo.

This patch sorts operands of comparisons by flipping opcode. For example, Above(0, @2) is
converted to Below(@2, 0). This sorting is the same to handleCommutativity rule. Since we
canonicalize comparisons to have constant value at least on the right hand side, we can
remove pattern matchings checking leftImm in B3LowerToAir.

Since this flipping changes the opcode of the value, to achieve safely, we just create a
new value which has flipped opcode and swapped operands. If we can fold it to a constant,
we replace m_value with this constant. If we fail to fold it to constant, we replace
m_value with the flipped one.

These comparisons are already handled in testb3.

• b3/B3LowerToAir.cpp:
• b3/B3ReduceStrength.cpp:

File:

• trunk/Source/JavaScriptCore/b3/B3ReduceStrength.cpp (modified) (3 diffs)

trunk/Source/JavaScriptCore/b3/B3ReduceStrength.cpp

@@ -1667 +1667 @@
 
         case LessThan:
-            // FIXME: We could do a better job of canonicalizing integer comparisons.
-            // https://bugs.webkit.org/show_bug.cgi?id=150958
-
-            replaceWithNewValue(
-                m_proc.addBoolConstant(
-                    m_value->origin(),
-                    m_value->child(0)->lessThanConstant(m_value->child(1))));
-            break;
-
         case GreaterThan:
-            replaceWithNewValue(
-                m_proc.addBoolConstant(
-                    m_value->origin(),
-                    m_value->child(0)->greaterThanConstant(m_value->child(1))));
-            break;
-
         case LessEqual:
-            replaceWithNewValue(
-                m_proc.addBoolConstant(
-                    m_value->origin(),
-                    m_value->child(0)->lessEqualConstant(m_value->child(1))));
-            break;
-
         case GreaterEqual:
-            replaceWithNewValue(
-                m_proc.addBoolConstant(
-                    m_value->origin(),
-                    m_value->child(0)->greaterEqualConstant(m_value->child(1))));
-            break;
-
         case Above:
-            replaceWithNewValue(
-                m_proc.addBoolConstant(
-                    m_value->origin(),
-                    m_value->child(0)->aboveConstant(m_value->child(1))));
-            break;
-
         case Below:
-            replaceWithNewValue(
-                m_proc.addBoolConstant(
-                    m_value->origin(),
-                    m_value->child(0)->belowConstant(m_value->child(1))));
-            break;
-
         case AboveEqual:
-            replaceWithNewValue(
-                m_proc.addBoolConstant(
-                    m_value->origin(),
-                    m_value->child(0)->aboveEqualConstant(m_value->child(1))));
-            break;
-
-        case BelowEqual:
-            replaceWithNewValue(
-                m_proc.addBoolConstant(
-                    m_value->origin(),
-                    m_value->child(0)->belowEqualConstant(m_value->child(1))));
-            break;
+        case BelowEqual: {
+            auto* value = newComparisonVaueIfNecessary();
+            TriState result = MixedTriState;
+            switch (value->opcode()) {
+            case LessThan:
+                result = value->child(1)->greaterThanConstant(value->child(0));
+                break;
+            case GreaterThan:
+                result = value->child(1)->lessThanConstant(value->child(0));
+                break;
+            case LessEqual:
+                result = value->child(1)->greaterEqualConstant(value->child(0));
+                break;
+            case GreaterEqual:
+                result = value->child(1)->lessEqualConstant(value->child(0));
+                break;
+            case Above:
+                result = value->child(1)->belowConstant(value->child(0));
+                break;
+            case Below:
+                result = value->child(1)->aboveConstant(value->child(0));
+                break;
+            case AboveEqual:
+                result = value->child(1)->belowEqualConstant(value->child(0));
+                break;
+            case BelowEqual:
+                result = value->child(1)->aboveEqualConstant(value->child(0));
+                break;
+            default:
+                RELEASE_ASSERT_NOT_REACHED();
+                break;
+            }
+
+            if (auto* constant = m_proc.addBoolConstant(value->origin(), result)) {
+                replaceWithNewValue(constant);
+                break;
+            }
+
+            // Replace with newly created "value". Its opcode is flipped and operands are swapped from m_value.
+            if (m_value != value)
+                replaceWithNewValue(value);
+            break;
+        }
 
         case EqualOrUnordered:
@@ -2135 +2127 @@
     }
 
+    static bool shouldSwapBinaryOperands(Value* value)
+    {
+        // Note that we have commutative operations that take more than two children. Those operations may
+        // commute their first two children while leaving the rest unaffected.
+        ASSERT(value->numChildren() >= 2);
+
+        // Leave it alone if the right child is a constant.
+        if (value->child(1)->isConstant()
+            || value->child(0)->opcode() == AtomicStrongCAS)
+            return false;
+
+        if (value->child(0)->isConstant())
+            return true;
+
+        if (value->child(1)->opcode() == AtomicStrongCAS)
+            return true;
+
+        // Sort the operands. This is an important canonicalization. We use the index instead of
+        // the address to make this at least slightly deterministic.
+        if (value->child(0)->index() > value->child(1)->index())
+            return true;
+
+        return false;
+    }
+
     // Turn this: Add(constant, value)
     // Into this: Add(value, constant)
@@ -2144 +2161 @@
     void handleCommutativity()
     {
-        // Note that we have commutative operations that take more than two children. Those operations may
-        // commute their first two children while leaving the rest unaffected.
-        ASSERT(m_value->numChildren() >= 2);
-        
-        // Leave it alone if the right child is a constant.
-        if (m_value->child(1)->isConstant()
-            || m_value->child(0)->opcode() == AtomicStrongCAS)
-            return;
-        
-        auto swap = [&] () {
+        if (shouldSwapBinaryOperands(m_value)) {
             std::swap(m_value->child(0), m_value->child(1));
             m_changed = true;
+        }
+    }
+
+    Value* newComparisonVaueIfNecessary()
+    {
+        auto flip = [] (Opcode opcode) {
+            switch (opcode) {
+            case LessThan:
+                return GreaterThan;
+            case GreaterThan:
+                return LessThan;
+            case LessEqual:
+                return GreaterEqual;
+            case GreaterEqual:
+                return LessEqual;
+            case Above:
+                return Below;
+            case Below:
+                return Above;
+            case AboveEqual:
+                return BelowEqual;
+            case BelowEqual:
+                return AboveEqual;
+            default:
+                return opcode;
+            }
         };
-        
-        if (m_value->child(0)->isConstant())
-            return swap();
-        
-        if (m_value->child(1)->opcode() == AtomicStrongCAS)
-            return swap();
-        
-        // Sort the operands. This is an important canonicalization. We use the index instead of
-        // the address to make this at least slightly deterministic.
-        if (m_value->child(0)->index() > m_value->child(1)->index())
-            return swap();
+        if (shouldSwapBinaryOperands(m_value)) {
+            m_changed = true;
+            return m_proc.add<Value>(flip(m_value->opcode()), m_value->origin(), m_value->child(1), m_value->child(0));
+        }
+        return m_value;
     }