698778 - IonMonkey: don't allocate a register for constant comparison operands

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

For an i < 10 loop condition we generate these instructions:

0x9ce167:	mov    $0xa,%ecx
0x9ce16c:	cmp    %ecx,%eax
0x9ce16e:	jl     0x9ce1a1

If we use $0xa directly we can improve register allocation and save an instruction.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

Add support for constant RHS to LCompareI(AndBranch). Also reorders comparisons like 10 > x to x < 10.

Tomorrow I will try to install a virtual ARM machine and attach a second patch for the ARM changes.

Comment 2

[Marty Rosenberg [:mjrosenb]]

While this feature is nice to have, "always put immediates in instructions" is not exactly an optimal strategy for arm, since only a limited range of values can be encoded in one instruction.
for example, If we want to add 0xf000f to r0 (0xf000f cannot be encoded as a single operand, nor as a halfword-move, and will require two instructions to add, and to move into a temp register, so the possible outputs are:
movw r10, 0xf
movt r10, 0xf0000
add r0, r10

If we try to encode this as a single instruction, we get a slightly better sequence,
add r0, 0xf
add r0, 0xf0000
however, if we need to add 0xf000f to r0, r1 and r2,
the 5 instruction sequence
movw r10, 0xf
movt r0, 0xf0000
add r0, r0, r10
add r1, r1, r10
add r2, r2, r10
will be better than the corresponding immediate encoded sequence:

add r0, 0xf
add r0, 0xf0000
add r1, 0xf
add r1, 0xf0000
add r2, 0xf
add r2, 0xf0000

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Marty Rosenberg [:Marty] from comment #2)
> While this feature is nice to have, "always put immediates in instructions"
> is not exactly an optimal strategy for arm, since only a limited range of
> values can be encoded in one instruction.

Interesting. Would it make sense to have useRegisterOrConstant return a register for such constants on ARM?

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 571149 [details] [diff] [review]
Patch

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

re: ARM, Marty would know best, specializing useRegisterOrConstant sounds fine though. Technically it embeds any Value * so if it comes to that, having useRegisterOrInteger might be better.

::: js/src/ion/LIR-Common.h
@@ +349,5 @@
>      LIR_HEADER(CompareI);
>      LCompareI(JSOp jsop, const LAllocation &left, const LAllocation &right)
>        : jsop_(jsop)
>      {
> +        JS_ASSERT(!left.isConstant());

nit: These asserts shouldn't be necessary, they'll be caught in codegen (and might not be true on other platforms).

Comment 5

[Jan de Mooij [:jandem]]

Attachment: ARM part

Also makes the order of ma_cmp arguments a bit more consistent, as we discussed yesterday.

Comment 6

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 575204 [details] [diff] [review]
ARM part

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

This all seems pretty straightforward.

Comment 7

[Jan de Mooij [:jandem]]

http://hg.mozilla.org/projects/ionmonkey/rev/63421e0a7c45

Marty, should we file a follow-up bug for ARM immediate vs. register improvements?