From a3da02d4fd818b5f73e3233ead24f33f31418701 Mon Sep 17 00:00:00 2001 From: Dan Gohman Date: Mon, 30 Jun 2014 09:10:25 -0700 Subject: [PATCH] Bug 999257 - IonMonkey: Commutate operands to expose more coalescing opportunities with loop phis r=nbp --- js/src/jit/Lowering.cpp | 81 +++++++++++++++++++++++++----------- js/src/jit/MIR.cpp | 17 ++++++++ js/src/jit/MIR.h | 22 ++++++++++ js/src/jit/RangeAnalysis.cpp | 14 ++----- 4 files changed, 99 insertions(+), 35 deletions(-) diff --git a/js/src/jit/Lowering.cpp b/js/src/jit/Lowering.cpp index 0db3721ad8343..44b23ca02a5e3 100644 --- a/js/src/jit/Lowering.cpp +++ b/js/src/jit/Lowering.cpp @@ -622,27 +622,58 @@ ReorderComparison(JSOp op, MDefinition **lhsp, MDefinition **rhsp) return op; } -static void -ReorderCommutative(MDefinition **lhsp, MDefinition **rhsp) +static bool +ShouldReorderCommutative(MDefinition *lhs, MDefinition *rhs, MInstruction *ins) { - MDefinition *lhs = *lhsp; - MDefinition *rhs = *rhsp; + // lhs and rhs are used by the commutative operator. + JS_ASSERT(lhs->hasDefUses()); + JS_ASSERT(rhs->hasDefUses()); // Ensure that if there is a constant, then it is in rhs. - // In addition, since clobbering binary operations clobber the left - // operand, prefer a non-constant lhs operand with no further uses. - if (rhs->isConstant()) - return; + return false; + if (lhs->isConstant()) + return true; - // lhs and rhs are used by the commutative operator. If they have any - // *other* uses besides, try to reorder to avoid clobbering them. To - // be fully precise, we should check whether this is the *last* use, - // but checking hasOneDefUse() is a decent approximation which doesn't - // require any extra analysis. - JS_ASSERT(lhs->hasDefUses()); - JS_ASSERT(rhs->hasDefUses()); - if (lhs->isConstant() || (rhs->hasOneDefUse() && !lhs->hasOneDefUse())) { + // Since clobbering binary operations clobber the left operand, prefer a + // non-constant lhs operand with no further uses. To be fully precise, we + // should check whether this is the *last* use, but checking hasOneDefUse() + // is a decent approximation which doesn't require any extra analysis. + bool rhsSingleUse = rhs->hasOneDefUse(); + bool lhsSingleUse = lhs->hasOneDefUse(); + if (rhsSingleUse) { + if (!lhsSingleUse) + return true; + } else { + if (lhsSingleUse) + return false; + } + + // If this is a reduction-style computation, such as + // + // sum = 0; + // for (...) + // sum += ...; + // + // put the phi on the left to promote coalescing. This is fairly specific. + if (rhsSingleUse && + rhs->isPhi() && + rhs->block()->isLoopHeader() && + ins == rhs->toPhi()->getLoopBackedgeOperand()) + { + return true; + } + + return false; +} + +static void +ReorderCommutative(MDefinition **lhsp, MDefinition **rhsp, MInstruction *ins) +{ + MDefinition *lhs = *lhsp; + MDefinition *rhs = *rhsp; + + if (ShouldReorderCommutative(lhs, rhs, ins)) { *rhsp = lhs; *lhsp = rhs; } @@ -828,7 +859,7 @@ LIRGenerator::visitTest(MTest *test) MDefinition *lhs = opd->getOperand(0); MDefinition *rhs = opd->getOperand(1); if (lhs->type() == MIRType_Int32 && rhs->type() == MIRType_Int32) { - ReorderCommutative(&lhs, &rhs); + ReorderCommutative(&lhs, &rhs, test); return lowerForBitAndAndBranch(new(alloc()) LBitAndAndBranch(ifTrue, ifFalse), test, lhs, rhs); } } @@ -1011,7 +1042,7 @@ LIRGenerator::lowerBitOp(JSOp op, MInstruction *ins) MDefinition *rhs = ins->getOperand(1); if (lhs->type() == MIRType_Int32 && rhs->type() == MIRType_Int32) { - ReorderCommutative(&lhs, &rhs); + ReorderCommutative(&lhs, &rhs, ins); return lowerForALU(new(alloc()) LBitOpI(op), ins, lhs, rhs); } @@ -1228,7 +1259,7 @@ LIRGenerator::visitMinMax(MMinMax *ins) MDefinition *first = ins->getOperand(0); MDefinition *second = ins->getOperand(1); - ReorderCommutative(&first, &second); + ReorderCommutative(&first, &second, ins); if (ins->specialization() == MIRType_Int32) { LMinMaxI *lir = new(alloc()) LMinMaxI(useRegisterAtStart(first), useRegisterOrConstant(second)); @@ -1390,7 +1421,7 @@ LIRGenerator::visitAdd(MAdd *ins) if (ins->specialization() == MIRType_Int32) { JS_ASSERT(lhs->type() == MIRType_Int32); - ReorderCommutative(&lhs, &rhs); + ReorderCommutative(&lhs, &rhs, ins); LAddI *lir = new(alloc()) LAddI; if (ins->fallible() && !assignSnapshot(lir, Bailout_OverflowInvalidate)) @@ -1405,13 +1436,13 @@ LIRGenerator::visitAdd(MAdd *ins) if (ins->specialization() == MIRType_Double) { JS_ASSERT(lhs->type() == MIRType_Double); - ReorderCommutative(&lhs, &rhs); + ReorderCommutative(&lhs, &rhs, ins); return lowerForFPU(new(alloc()) LMathD(JSOP_ADD), ins, lhs, rhs); } if (ins->specialization() == MIRType_Float32) { JS_ASSERT(lhs->type() == MIRType_Float32); - ReorderCommutative(&lhs, &rhs); + ReorderCommutative(&lhs, &rhs, ins); return lowerForFPU(new(alloc()) LMathF(JSOP_ADD), ins, lhs, rhs); } @@ -1460,7 +1491,7 @@ LIRGenerator::visitMul(MMul *ins) if (ins->specialization() == MIRType_Int32) { JS_ASSERT(lhs->type() == MIRType_Int32); - ReorderCommutative(&lhs, &rhs); + ReorderCommutative(&lhs, &rhs, ins); // If our RHS is a constant -1 and we don't have to worry about // overflow, we can optimize to an LNegI. @@ -1471,7 +1502,7 @@ LIRGenerator::visitMul(MMul *ins) } if (ins->specialization() == MIRType_Double) { JS_ASSERT(lhs->type() == MIRType_Double); - ReorderCommutative(&lhs, &rhs); + ReorderCommutative(&lhs, &rhs, ins); // If our RHS is a constant -1.0, we can optimize to an LNegD. if (rhs->isConstant() && rhs->toConstant()->value() == DoubleValue(-1.0)) @@ -1481,7 +1512,7 @@ LIRGenerator::visitMul(MMul *ins) } if (ins->specialization() == MIRType_Float32) { JS_ASSERT(lhs->type() == MIRType_Float32); - ReorderCommutative(&lhs, &rhs); + ReorderCommutative(&lhs, &rhs, ins); // We apply the same optimizations as for doubles if (rhs->isConstant() && rhs->toConstant()->value() == Float32Value(-1.0f)) diff --git a/js/src/jit/MIR.cpp b/js/src/jit/MIR.cpp index dfb326baa7805..df495cb576e9d 100644 --- a/js/src/jit/MIR.cpp +++ b/js/src/jit/MIR.cpp @@ -909,6 +909,23 @@ MTypeBarrier::printOpcode(FILE *fp) const getOperand(0)->printName(fp); } +#ifdef DEBUG +void +MPhi::assertLoopPhi() const +{ + // getLoopPredecessorOperand and getLoopBackedgeOperand rely on these + // predecessors being at indices 0 and 1. + MBasicBlock *pred = block()->getPredecessor(0); + MBasicBlock *back = block()->getPredecessor(1); + JS_ASSERT(pred == block()->loopPredecessor()); + JS_ASSERT(pred->successorWithPhis() == block()); + JS_ASSERT(pred->positionInPhiSuccessor() == 0); + JS_ASSERT(back == block()->backedge()); + JS_ASSERT(back->successorWithPhis() == block()); + JS_ASSERT(back->positionInPhiSuccessor() == 1); +} +#endif + void MPhi::removeOperand(size_t index) { diff --git a/js/src/jit/MIR.h b/js/src/jit/MIR.h index 5ed6695e7874d..54c29c3468b87 100644 --- a/js/src/jit/MIR.h +++ b/js/src/jit/MIR.h @@ -5026,6 +5026,28 @@ class MPhi MOZ_FINAL : public MDefinition, public InlineListNode } bool specializeType(); +#ifdef DEBUG + // Assert that this is a phi in a loop header with a unique predecessor and + // a unique backedge. + void assertLoopPhi() const; +#else + void assertLoopPhi() const {} +#endif + + // Assuming this phi is in a loop header with a unique loop entry, return + // the phi operand along the loop entry. + MDefinition *getLoopPredecessorOperand() const { + assertLoopPhi(); + return getOperand(0); + } + + // Assuming this phi is in a loop header with a unique loop entry, return + // the phi operand along the loop backedge. + MDefinition *getLoopBackedgeOperand() const { + assertLoopPhi(); + return getOperand(1); + } + // Whether this phi's type already includes information for def. bool typeIncludes(MDefinition *def); diff --git a/js/src/jit/RangeAnalysis.cpp b/js/src/jit/RangeAnalysis.cpp index 10379f724ff3f..80707a4d8a7de 100644 --- a/js/src/jit/RangeAnalysis.cpp +++ b/js/src/jit/RangeAnalysis.cpp @@ -1723,13 +1723,13 @@ RangeAnalysis::analyzeLoopIterationCount(MBasicBlock *header, // The first operand of the phi should be the lhs' value at the start of // the first executed iteration, and not a value written which could // replace the second operand below during the middle of execution. - MDefinition *lhsInitial = lhs.term->toPhi()->getOperand(0); + MDefinition *lhsInitial = lhs.term->toPhi()->getLoopPredecessorOperand(); if (lhsInitial->block()->isMarked()) return nullptr; // The second operand of the phi should be a value written by an add/sub // in every loop iteration, i.e. in a block which dominates the backedge. - MDefinition *lhsWrite = lhs.term->toPhi()->getOperand(1); + MDefinition *lhsWrite = lhs.term->toPhi()->getLoopBackedgeOperand(); if (lhsWrite->isBeta()) lhsWrite = lhsWrite->getOperand(0); if (!lhsWrite->isAdd() && !lhsWrite->isSub()) @@ -1809,17 +1809,11 @@ RangeAnalysis::analyzeLoopPhi(MBasicBlock *header, LoopIterationBound *loopBound JS_ASSERT(phi->numOperands() == 2); - MBasicBlock *preLoop = header->loopPredecessor(); - JS_ASSERT(!preLoop->isMarked() && preLoop->successorWithPhis() == header); - - MBasicBlock *backedge = header->backedge(); - JS_ASSERT(backedge->isMarked() && backedge->successorWithPhis() == header); - - MDefinition *initial = phi->getOperand(preLoop->positionInPhiSuccessor()); + MDefinition *initial = phi->getLoopPredecessorOperand(); if (initial->block()->isMarked()) return; - SimpleLinearSum modified = ExtractLinearSum(phi->getOperand(backedge->positionInPhiSuccessor())); + SimpleLinearSum modified = ExtractLinearSum(phi->getLoopBackedgeOperand()); if (modified.term != phi || modified.constant == 0) return;