The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 571149 [details] [diff] [review]
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.
Attachment #571149 - Flags: review?(dvander)
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
(Assignee)

Comment 3

6 years ago
(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 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).
Attachment #571149 - Flags: review?(dvander) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 575204 [details] [diff] [review]
ARM part

Also makes the order of ma_cmp arguments a bit more consistent, as we discussed yesterday.
Attachment #575204 - Flags: review?(mrosenberg)
Comment on attachment 575204 [details] [diff] [review]
ARM part

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

This all seems pretty straightforward.
Attachment #575204 - Flags: review?(mrosenberg) → review+
(Assignee)

Comment 7

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

Marty, should we file a follow-up bug for ARM immediate vs. register improvements?
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.