Last Comment Bug 698778 - IonMonkey: don't allocate a register for constant comparison operands
: IonMonkey: don't allocate a register for constant comparison operands
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
:
Mentors:
Depends on:
Blocks: IonMonkey
  Show dependency treegraph
 
Reported: 2011-11-01 09:43 PDT by Jan de Mooij [:jandem]
Modified: 2011-11-18 09:48 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (6.23 KB, patch)
2011-11-01 14:57 PDT, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Splinter Review
ARM part (17.45 KB, patch)
2011-11-17 08:37 PST, Jan de Mooij [:jandem]
marty.rosenberg: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2011-11-01 09:43:44 PDT
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] 2011-11-01 14:57:38 PDT
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.
Comment 2 Marty Rosenberg [:mjrosenb] 2011-11-07 05:59:38 PST
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] 2011-11-07 07:29:55 PST
(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] 2011-11-07 08:50:09 PST
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] 2011-11-17 08:37:17 PST
Created attachment 575204 [details] [diff] [review]
ARM part

Also makes the order of ma_cmp arguments a bit more consistent, as we discussed yesterday.
Comment 6 Marty Rosenberg [:mjrosenb] 2011-11-18 05:40:12 PST
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] 2011-11-18 09:48:58 PST
http://hg.mozilla.org/projects/ionmonkey/rev/63421e0a7c45

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

Note You need to log in before you can comment on or make changes to this bug.