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.

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.

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

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.

(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).

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 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 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 :)

Carry review from previous patch.

Carry review from previous patch.

Carry review from previous patch.

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.

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

Quick follow-up -- my mind is blergh. https://hg.mozilla.org/integration/mozilla-inbound/rev/d557bcfcecb0

Patchset broke builds with --disable-unified-compilation. Fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/bf1c135138c4

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

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