985876 - Ionmonkey: Refactor Ionmonkey shared code to support MIPS backend

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Branislav Rankov [:rankov]]

There are few issues in shared code that are not compatible with MIPS. These issues are mostly local to IonMacroAssembler, BaselineIC and CodeGenarator. The main problem is that MIPS doesn't support flags. Because of this cmp32(), cmpPtr(), j() and other instructions cannot be used in shared code. 

I plan to fix this by replacing pairs of cmp32() and j() with branch32(). And similar. 

Also, I plan to change bailoutIf to bailoutCmp32, bailoutCmpPtr, bailoutTest32 and similar.

Comment 1

[Branislav Rankov [:rankov]]

Attachment: Shared-code-refactoring.patch

This is refactoring for X86, X64 and ARM code. Refactoring of the MIPS code will be added in a separate patch to make reviewing easier.

Comment 2

[Branislav Rankov [:rankov]]

Attachment: Shared-code-refactoring.patch

Just realized that the piece of the patch was missing. Fixed that.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8394798 [details] [diff] [review]
Shared-code-refactoring.patch

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

This looks reasonable at first glance but see comments below. Could you please split this up in multiple smaller patches? It's a lot easier to review 20 small patches than one big one. For instance, moving clampDoubleToUint8 to platform-specific code is really nice, but that could have its own patch :)

Also, please add the following to your .hgrc, assuming you use hg to generate the diff:

[diff]
git = 1
showfunc = true
unified = 8

It's much easier to review when you see the function name and more lines of context :)

::: js/src/jit/BaselineCompiler.cpp
@@ +341,2 @@
>              masm.j(Assembler::NonZero, &pushLoop);
> +#else

I think it's a bit cleaner to add a branchSub32 so that you can do:

masm.branchSub32(Assembler::NonZero, Imm32(LOOP_UNROLL_FACTOR), R1.scratchReg(), &pushLoop);

::: js/src/jit/CodeGenerator.cpp
@@ +2392,5 @@
>      if (!apply->hasSingleTarget()) {
>          masm.loadObjClass(calleereg, objreg);
> +
> +        if (!bailoutCmpPtr(Assembler::NotEqual, objreg, ImmPtr(&JSFunction::class_),
> +                           apply->snapshot()))

Add {}

@@ +4066,5 @@
>      masm.unboxInt32(Address(temp, ArrayBufferObject::flagsOffset()), temp);
>      masm.and32(Imm32(ArrayBufferObject::neuteredFlag()), temp);
>  
> +#ifdef JS_CODEGEN_MIPS
> +    if (!bailoutCmpPtr(Assembler::NonZero, temp, temp, lir->snapshot()))

bailoutTestPtr

@@ +4106,3 @@
>      Label done;
> +    Assembler::Condition cond = ins->mir()->isMax() ? Assembler::GreaterThan :
> +                                Assembler::LessThan;

Format like this (? and : under ins):

    Assembler::Condition cond = ins->mir()->isMax()
                                ? Assembler::GreaterThan
                                : Assembler::GreaterThan;

@@ +4132,3 @@
>      masm.neg32(input);
> +#ifdef JS_CODEGEN_MIPS
> +    if (ins->snapshot() && !bailoutCmpPtr(Assembler::Equal, input, Imm32(INT32_MIN),

Add bailoutCmp32

@@ +4823,5 @@
>      masm.addPtr(Imm32(2), from);
>      masm.addPtr(Imm32(2), to);
>      masm.sub32(Imm32(1), len);
> +#ifdef JS_CODEGEN_MIPS
> +    masm.branch32(Assembler::NonZero, len, len, &start);

Use branchSub32 I mentioned in another comment.

@@ +5213,5 @@
>      // would succeed on any nonnegative index).
>      if (max != 0) {
> +        if (max < 0) {
> +            Label bail;
> +            masm.add32TestOverflow(Imm32(max), temp, &bail);

If we add branchAdd32, we can do branchAdd32(Assembler::Overflow, ...) and use that function for cases other than overflow.

Comment 4

[Branislav Rankov [:rankov]]

Attachment: Move-to-architecture-specific.patch

Comment 5

[Branislav Rankov [:rankov]]

Attachment: Refactor-MacroAssembler-Baseline.patch

Comment 6

[Branislav Rankov [:rankov]]

Attachment: Refactor-CodeGenerator.patch

Comment 7

[Branislav Rankov [:rankov]]

(In reply to Jan de Mooij [:jandem] from comment #3)

I have updated the places you requested. Also I have split the patch into 3 smaller ones.

Comment 8

[Sean Stangl [:sstangl]]

Comment on attachment 8400622 [details] [diff] [review]
Move-to-architecture-specific.patch

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

Breaking up the patch does make it much more pleasant to review. Thanks for doing that.

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +2444,5 @@
> +        // If the lower 32 bits of the double were 0, then this was an exact number,
> +        // and it should be even.
> +        ma_bic(Imm32(1), output, NoSetCond, Zero);
> +        bind(&notSplit);
> +

nit: vertical whitespace here can be removed (it was in the original version too, but might as well clean it up now).

Comment 9

[Sean Stangl [:sstangl]]

Comment on attachment 8400623 [details] [diff] [review]
Refactor-MacroAssembler-Baseline.patch

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

::: js/src/jit/BaselineIC.cpp
@@ +2198,5 @@
>  
>          // Compare payload regs of R0 and R1.
>          Assembler::Condition cond = JSOpToCondition(op_, /* signed = */true);
> +        masm.cmp32Set(cond, lhsIsInt32_ ? int32Reg : boolReg,
> +                      lhsIsInt32_ ? boolReg : int32Reg, R0.scratchReg());

I tend to prefer parentheses around subexpressions like these: (lhsIsInt32_ ? int32Reg : boolReg). Try it and see if you think it reads more nicely.

Comment 10

[Sean Stangl [:sstangl]]

Comment on attachment 8400625 [details] [diff] [review]
Refactor-CodeGenerator.patch

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

lgtm. Just some style nits.

::: js/src/jit/CodeGenerator.cpp
@@ +4151,5 @@
>      return true;
>  }
>  
> +
> +

Whitespace should be reduced to one line.

@@ +4164,4 @@
>      masm.neg32(input);
> +#ifdef JS_CODEGEN_MIPS
> +    if (ins->snapshot() && !bailoutCmp32(Assembler::Equal, input, Imm32(INT32_MIN),
> +                                          ins->snapshot()))

This would look better if we stored ins->snapshot() in a local variable, and then had the entire if() condition on a single line.

Although this is no longer applicable, SpiderMonkey style guide requires braces for the body of a conditional if the condition spans multiple lines.

@@ +5290,2 @@
>      Assembler::Condition cond;
>      if (index->isConstant())

Because the body of the conditional is now multiple lines, both the main path and the else path require {} braces:

> if (foo) {
>   x;
>   y;
> } else {
>   z;
> }

::: js/src/jit/arm/CodeGenerator-arm.h
@@ +75,5 @@
>      // true, and the false block if |cond| is false.
>      void emitBranch(Assembler::Condition cond, MBasicBlock *ifTrue, MBasicBlock *ifFalse);
>  
> +    void testNullEmitBranch(Assembler::Condition cond, const ValueOperand &value,
> +                            MBasicBlock *ifTrue, MBasicBlock *ifFalse) {

{ on new line

@@ +80,5 @@
> +        cond = masm.testNull(cond, value);
> +        emitBranch(cond, ifTrue, ifFalse);
> +    }
> +    void testUndefinedEmitBranch(Assembler::Condition cond, const ValueOperand &value,
> +                            MBasicBlock *ifTrue, MBasicBlock *ifFalse) {

{ on new line

::: js/src/jit/shared/CodeGenerator-x86-shared.h
@@ +86,5 @@
>                      Assembler::NaNCond ifNaN = Assembler::NaN_HandledByCond);
>      void emitBranch(Assembler::DoubleCondition cond, MBasicBlock *ifTrue, MBasicBlock *ifFalse);
>  
> +    void testNullEmitBranch(Assembler::Condition cond, const ValueOperand &value,
> +                            MBasicBlock *ifTrue, MBasicBlock *ifFalse) {

{ on new line, because of multi-line arguments

@@ +91,5 @@
> +        cond = masm.testNull(cond, value);
> +        emitBranch(cond, ifTrue, ifFalse);
> +    }
> +    void testUndefinedEmitBranch(Assembler::Condition cond, const ValueOperand &value,
> +                            MBasicBlock *ifTrue, MBasicBlock *ifFalse) {

{ on new line

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8400622 [details] [diff] [review]
Move-to-architecture-specific.patch

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

Thanks for splitting it up. Resetting review as Sean reviewed this already :)

Comment 12

[Branislav Rankov [:rankov]]

Attachment: Move-to-architecture-specific.patch

Carry review from previous patch.

Comment 13

[Branislav Rankov [:rankov]]

Attachment: Refactor-MacroAssembler-Baseline.patch

Carry review from previous patch.

Comment 14

[Branislav Rankov [:rankov]]

Attachment: Refactor-CodeGenerator.patch

Carry review from previous patch.

Comment 15

[Branislav Rankov [:rankov]]

I have fixed all the issues form the comments above.

(In reply to Sean Stangl [:sstangl] from comment #9)

> I tend to prefer parentheses around subexpressions like these: (lhsIsInt32_
> ? int32Reg : boolReg). Try it and see if you think it reads more nicely.

It does look better that way. :)

Comment 16

[Branislav Rankov [:rankov]]

Jan, Sean, can one of you commit these patches?

Make sure they go in the correct order:
1. Move-to-architecture-specific.patch
2. Refactor-MacroAssembler-Baseline.patch
3. Refactor-CodeGenerator.patch

Comment 17

[Sean Stangl [:sstangl]]

Comment on attachment 8401389 [details] [diff] [review]
Refactor-CodeGenerator.patch

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

::: js/src/jit/CodeGenerator.cpp
@@ +4100,5 @@
>  
>      masm.extractObject(Address(obj, TypedObject::ownerOffset()), temp);
>      masm.unboxInt32(Address(temp, ArrayBufferObject::flagsOffset()), temp);
> +
> +    if (!bailoutTestPtr(Assembler::NonZero, temp, temp, lir->snapshot()))

This must be bailoutTest32(), and must test only the neuteredFlag bit. I can fix locally; no need to re-upload.

Comment 18

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/239013bcaf7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ec0a91f244f
https://hg.mozilla.org/integration/mozilla-inbound/rev/68d5ad5a7f7b

Comment 19

[Sean Stangl [:sstangl]]

Quick follow-up -- my mind is blergh.

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

Comment 20

[Sean Stangl [:sstangl]]

Patchset broke builds with --disable-unified-compilation. Fix:

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

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/239013bcaf7f
https://hg.mozilla.org/mozilla-central/rev/2ec0a91f244f
https://hg.mozilla.org/mozilla-central/rev/68d5ad5a7f7b
https://hg.mozilla.org/mozilla-central/rev/d557bcfcecb0
https://hg.mozilla.org/mozilla-central/rev/bf1c135138c4

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

This introduced a warning that cond is unused. KILL IT WITH FIRE (r=nbp over irc):
https://hg.mozilla.org/integration/mozilla-inbound/rev/17a33fc3d9a4

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/17a33fc3d9a4