Title: [229513] trunk/Source/_javascript_Core
- Revision
- 229513
- Author
- [email protected]
- Date
- 2018-03-10 23:16:15 -0800 (Sat, 10 Mar 2018)
Log 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:
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (229512 => 229513)
--- trunk/Source/_javascript_Core/ChangeLog 2018-03-11 01:49:00 UTC (rev 229512)
+++ trunk/Source/_javascript_Core/ChangeLog 2018-03-11 07:16:15 UTC (rev 229513)
@@ -1,3 +1,25 @@
+2018-03-10 Yusuke Suzuki <[email protected]>
+
+ 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:
+
2018-03-09 Mark Lam <[email protected]>
offlineasm should reset the Assembler's working state before doing another pass for a new target.
Modified: trunk/Source/_javascript_Core/b3/B3LowerToAir.cpp (229512 => 229513)
--- trunk/Source/_javascript_Core/b3/B3LowerToAir.cpp 2018-03-11 01:49:00 UTC (rev 229512)
+++ trunk/Source/_javascript_Core/b3/B3LowerToAir.cpp 2018-03-11 07:16:15 UTC (rev 229513)
@@ -1462,11 +1462,6 @@
Value* right = value->child(1);
if (isInt(value->child(0)->type())) {
- // FIXME: We wouldn't have to worry about leftImm if we canonicalized integer
- // comparisons.
- // https://bugs.webkit.org/show_bug.cgi?id=150958
-
- Arg leftImm = imm(left);
Arg rightImm = imm(right);
auto tryCompare = [&] (
@@ -1486,12 +1481,6 @@
return result;
}
}
- if (leftImm && leftImm.isRepresentableAs(width, signedness)) {
- if (Inst result = tryCompare(width, leftImm, loadPromise(right, loadOpcode))) {
- commitInternal(right);
- return result;
- }
- }
return Inst();
};
@@ -1548,11 +1537,6 @@
// Now handle compares that involve an immediate and a tmp.
- if (leftImm && leftImm.isRepresentableAs<int32_t>()) {
- if (Inst result = tryCompare(width, leftImm, tmpPromise(right)))
- return result;
- }
-
if (rightImm && rightImm.isRepresentableAs<int32_t>()) {
if (Inst result = tryCompare(width, tmpPromise(left), rightImm))
return result;
Modified: trunk/Source/_javascript_Core/b3/B3ReduceStrength.cpp (229512 => 229513)
--- trunk/Source/_javascript_Core/b3/B3ReduceStrength.cpp 2018-03-11 01:49:00 UTC (rev 229512)
+++ trunk/Source/_javascript_Core/b3/B3ReduceStrength.cpp 2018-03-11 07:16:15 UTC (rev 229513)
@@ -1666,63 +1666,55 @@
break;
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: {
+ 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;
+ }
- case BelowEqual:
- replaceWithNewValue(
- m_proc.addBoolConstant(
- m_value->origin(),
- m_value->child(0)->belowEqualConstant(m_value->child(1))));
+ 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:
handleCommutativity();
@@ -2134,6 +2126,31 @@
predecessor->updatePredecessorsAfter();
}
+ 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)
//
@@ -2143,30 +2160,41 @@
// If we decide that value2 coming first is the canonical ordering.
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;
}
// FIXME: This should really be a forward analysis. Instead, we uses a bounded-search backwards
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
