773666 - IonMonkey: reconstruct operands after add/sub overflows

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: WIP (dc54ec097421)

Currently, when an add or sub overflows ion takes a bailout which needs to keep everything in the last resume point live, including everything which determined the two operands.  This leads to extra moves and register pressure to make sure that both operands of the add are in registers or memory distinct from the dest register of the add instruction itself.  This isn't really necessary, as if an add overflows and the src/dest are distinct registers then a sub instruction can recover the original values of the operands.

The attached patch makes this change, though still missing correct recovery after taking the overflow bailout.  On this benchmark:

function foo()
{
  var res = 0;
  for (var j = 0; j < 10000; j++)
    res += j;
}

for (var i = 0; i < 10000; i++)
  foo();

Improves --ion -n from 187ms to 150ms.  -m -n is 115ms, (old) d8 is 147ms.  Removing the overflow check on j++, as -m -n does, should take time down to that ballpark.

The main thing I'm concerned about with this patch is that it adds new resume points after arithmetic operations, even if they are not effectful.  While these resume points are not necessary for correctness, if we try to resume at a point earlier than an add from a point after the add, then the operands of the add are still held live across the add and need to be in separate registers.  Not sure yet the impacts of this change, how it affects other optimizations.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

Working patch.  When applied on top of the bug 814997 patches, for binary add/sub this eliminates all uses of the inputs for add/sub operations that were previously kept alive by safepoints on the add/sub or later operations.  Thus, the register allocator does not need to make copies of those inputs before the add/sub, eliminating unnecessary moves.

This works by ensuring that, when possible, add/sub instructions use an OOL code block to recover the original values of their inputs before bailing out, and mark those inputs in snapshots to the instruction so that the regalloc does not need to treat them as uses.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 685971 [details] [diff] [review]
patch

Review of attachment 685971 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments below addressed.

::: js/src/ion/LinearScan.cpp
@@ +327,5 @@
>      for (UsePositionIterator usePos(usesBegin()); usePos != usesEnd(); usePos++) {
> +        if (usePos->pos >= after) {
> +            LUse::Policy policy = usePos->use->policy();
> +            if (policy != LUse::KEEPALIVE && policy != LUse::RECOVERED_INPUT)
> +                return *usePos;

The "if (policy == LUse::RECOVERED_INPUT) continue;" you added below should ensure we never get a use position with this policy. So maybe just turn it into an assert:

JS_ASSERT(usePos->use->policy() != LUse::RECOVERED_INPUT);

@@ +1136,5 @@
>                  spillFrom = interval->getAllocation();
>              }
>  
> +            if ((reg->ins()->isAddI() && reg->ins()->toAddI()->recoversInput()) ||
> +                (reg->ins()->isSubI() && reg->ins()->toSubI()->recoversInput()))

Can you add a "virtual bool recoversInput()" to LInstruction which just returns false? Then this can be just

if (reg->ins->recoversInput())

::: js/src/ion/Lowering.cpp
@@ +809,5 @@
>              return false;
>  
> +        if (!lowerForALU(lir, ins, lhs, rhs))
> +            return false;
> +        if (ins->fallible() && lhs != rhs && lir->output()->policy() == LDefinition::MUST_REUSE_INPUT) {

The problem with lhs != rhs is that different MIR instructions may be assigned the same virtual register (see redefine/defineAs). So it's safer to compare lir->lhs()->toUse()->virtualRegister() to the rhs vreg.

@@ +816,5 @@
> +            // of that input does not need to be kept alive in the snapshot
> +            // for the instruction.
> +            lir->setRecoversInput();
> +            lir->snapshot()->rewriteRecoveredInput(*lir->getOperand(lir->output()->getReusedInput())->toUse());
> +        }

Would be good to add these lines to a separate "static void MaybeSetRecoversInput(LInstruction *lir)" or something.

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +577,5 @@
> +
> +    LAllocation *lhs = ins->getOperand(0);
> +    LAllocation *rhs = ins->getOperand(1);
> +
> +    JS_ASSERT(reg == ToRegister(lhs));

This should assert rhs does not use reg. Something like

JS_ASSERT_IF(rhs->isGeneralReg(), rhs->toGeneralReg()->reg() != reg);

Comment 3

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff12b7de0b4f

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ff12b7de0b4f