Closed Bug 773666 Opened 11 years ago Closed 10 years ago

IonMonkey: reconstruct operands after add/sub overflows

Categories

(Core :: JavaScript Engine, defect)

Other Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ion:t])

Attachments

(1 file, 1 obsolete file)

Attached patch WIP (dc54ec097421) (obsolete) — Splinter Review
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.
Depends on: 814997
Attached patch patchSplinter Review
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.
Assignee: general → bhackett1024
Attachment #641922 - Attachment is obsolete: true
Attachment #685971 - Flags: review?(jdemooij)
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);
Attachment #685971 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/ff12b7de0b4f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.