1123874 - Optimize MoveGroups with multiple targets for the same source

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Sometimes, the register allocator generates move groups which move the same source to multiple different targets.  This seems to happen a lot more often with the backtracking allocator than with LSRA, probably because the backtracking allocator has the flexibility to represent a vreg in multiple locations at once.

The move resolver doesn't try to optimize these moves at all though, and we end up generating dumb code like:

[MoveGroup]
movl       0xc0(%esp), %esi
push       0xc0(%esp)
pop        0x8c(%esp)

This appears in a hot loop in octane-mandreel on x86.

The attached patch improves the move resolver to look for moves with the same source, where that source is in memory and one of the targets is a register.  It then fiddles with the moves so that we end up with code for the above like:

[MoveGroup]
movl       0xc0(%esp), %esi
movl       %esi, 0x8c(%esp)

On x86 with the backtracking allocator, this improves my octane-mandreel score from 25k to 28k.  LSRA is 30k.

Even before this patch, backtracking generates code with fewer MoveGroup instructions (19% of total executed instructions vs. 21% for LSRA).  However, I think that memory->memory moves like the push/pop instructions above are poisonous for performance.  Before this patch the backtracking allocator generated more than 10 times as many of these memory push/pop instructions as LSRA.  This patch removes more than half of them but there is still work to be done in minimizing these (either by more MoveGroup optimizations or by improving the allocator itself).

Comment 1

[Dan Gohman [:sunfish]]

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

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

::: js/src/jit/MoveResolver.cpp
@@ +191,5 @@
> +            !existing.isCycleEnd())
> +        {
> +            MoveOp *after = (size_t(i + 1) < orderedMoves_.length())
> +                            ? &orderedMoves_[i + 1]
> +                            : orderedMoves_.end();

I think this would be easier to read as "orderedMoves_.begin() + i + 1", but either way is ok with me.

Comment 2

[Brian Hackett [Laid off!]]

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

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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