Closed Bug 985876 Opened 6 years ago Closed 6 years ago

Ionmonkey: Refactor Ionmonkey shared code to support MIPS backend

Categories

(Core :: JavaScript Engine: JIT, enhancement)

Other
Linux
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: rankov, Assigned: rankov)

References

Details

Attachments

(3 files, 5 obsolete files)

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.
Blocks: 969375
Assignee: nobody → branislav.rankov
Attached patch Shared-code-refactoring.patch (obsolete) — Splinter Review
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.
Attached patch Shared-code-refactoring.patch (obsolete) — Splinter Review
Just realized that the piece of the patch was missing. Fixed that.
Attachment #8394756 - Attachment is obsolete: true
Attachment #8394798 - Flags: review?(nicolas.b.pierron)
Attachment #8394798 - Flags: review?(sstangl)
Attachment #8394798 - Flags: review?(nicolas.b.pierron)
Attachment #8394798 - Flags: review?(jdemooij)
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.
Attachment #8394798 - Flags: review?(jdemooij)
Attachment #8394798 - Attachment is obsolete: true
Attachment #8394798 - Flags: review?(sstangl)
Attachment #8400622 - Flags: review?(sstangl)
Attachment #8400622 - Flags: review?(jdemooij)
Attachment #8400623 - Flags: review?(sstangl)
Attachment #8400623 - Flags: review?(jdemooij)
Attached patch Refactor-CodeGenerator.patch (obsolete) — Splinter Review
Attachment #8400625 - Flags: review?(sstangl)
Attachment #8400625 - Flags: review?(jdemooij)
(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 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).
Attachment #8400622 - Flags: review?(sstangl) → review+
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.
Attachment #8400623 - Flags: review?(sstangl) → review+
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
Attachment #8400625 - Flags: review?(sstangl) → review+
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 :)
Attachment #8400622 - Flags: review?(jdemooij)
Attachment #8400623 - Flags: review?(jdemooij)
Attachment #8400625 - Flags: review?(jdemooij)
Carry review from previous patch.
Attachment #8400622 - Attachment is obsolete: true
Attachment #8401387 - Flags: review+
Carry review from previous patch.
Attachment #8400623 - Attachment is obsolete: true
Attachment #8401388 - Flags: review+
Carry review from previous patch.
Attachment #8400625 - Attachment is obsolete: true
Attachment #8401389 - Flags: review+
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. :)
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 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.
Patchset broke builds with --disable-unified-compilation. Fix:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf1c135138c4
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
Duplicate of this bug: 941748
You need to log in before you can comment on or make changes to this bug.