We need to replace primtive function calls by inlined assembly versions. JaëgerMonkey implements them in mjit::Compiler::inlineNativeFunction (methodjit/FastBuiltins.cpp) This work is made on top of the modification of getprop made in Bug 715511, such as access to primitive functions are replaced by their corresponding constant. This bug will instrument calls check for known optimized versions of primitive and substitute the call by the corresponding MIR.

Cool, we really need this for Kraken gaussian-blur. IIRC, inlining Math.abs made JM+TI > 5x faster on that test.

Check type inferences types to make sure there is only one function useda Check arguments types and discard PassArgs from the MIRGraph stack, when an optimized version is found. Currently only Math.floor and Math.round are optimized by this patch, which should already improve the performances of sunspider/string-validate-input.js which is using Math.floor. This patch rely on a (small) subset of getprop optimization to remove MCallGetProperty which is always failing with primitive types because of the unbox(box(X), Object) trick, when X is a String.

Comment on attachment 591797 [details] [diff] [review] Inline native calls. (Math.abs, Math.round, Math.floor) Review of attachment 591797 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonBuilder.h @@ +305,5 @@ > bool jsop_setprop(JSAtom *atom); > bool jsop_newarray(uint32 count); > bool jsop_this(); > > + /* Optimizing */ nit: This comment should be elaborated or removed. ::: js/src/ion/MCallOptimize.cpp @@ +54,5 @@ > + // Copy PassArg arguments from ArgN to This. > + for (int32 i = argc; i >= 0; i--) { > + MPassArg *passArg = current->pop()->toPassArg(); > + JS_ASSERT(passArg->useCount() == 0); > + passArg->block()->discard(passArg); Here, you want: passArg->replaceAllUsesWith(wrapped); passArg->remove(passArg); and remove the call to the discard. remove() removes it from use chains, whichis needed here since the PassArg could be captured in resume points. @@ +64,5 @@ > + current->pop(); > + // FIXME: Why is the function useCount != 0 ? > + // MInstruction *fun = current->pop()->toInstruction(); > + // JS_ASSERT(fun->useCount() == 0); > + // fun->block()->discard(fun); (answered above) @@ +97,5 @@ > + MIRType thisType = MIRTypeFromValueType(thisTypes->getKnownTypeTag(cx)); > + MIRType arg0Type = thisType; > + if (argc == 0) { > + return OptimizingStatus_None; > + } No braces needed here. ::: js/src/ion/MIR.h @@ +988,5 @@ > : public MVariadicInstruction, > public CallPolicy > { > + public: > + struct SpecializeInfo { This structure isn't used in this patch, and it feels a little awkward. If it's used in a later patch, let's wait to see how it gets used there, and avoid checking it in early. ::: js/src/ion/arm/CodeGenerator-arm.cpp @@ +865,5 @@ > + return false; > + > + // input =< 0 > + masm.bind(&belowZero); > + masm.subsd(input, ScratchFloatReg); subsd (and addsd above) are x86-specific right? you might want Marty to review the arm parts. ::: js/src/ion/shared/CodeGenerator-x86-shared.cpp @@ +843,5 @@ > + Label belowZero, end; > + > + // if (masm.HasSSE41()) { > + // // FIXME use roundsd > + // } Remove this commented code.

Comment on attachment 591798 [details] [diff] [review] Inline native calls [2] (String.charAt, String.charCodeAt, String.fromCharCode) Review of attachment 591798 [details] [diff] [review]: ----------------------------------------------------------------- Looks good overall, just a few things that should be addressed first: ::: js/src/ion/CodeGenerator.cpp @@ +828,5 @@ > + masm.testl(output, Imm32(JSString::ROPE_BIT)); > + masm.j(Assembler::NotEqual, ool->entry()); > + masm.bind(ool->rejoin()); > + > + masm.lshiftPtr(Imm32(1), index); Somewhere in here you need a guard for the string length. The guard has to be a bailout, since charCodeAt(>= .length) returns... NaN :) Ideally though, you could emit an MBoundsCheck(index, MStringLength) or something. @@ +835,5 @@ > + masm.ma_ldrh(EDtrAddr(output, EDtrOffReg(index)), output); > +#else > + masm.addPtr(index, output); > + masm.load16(Address(output, 0), output); > +#endif Let's find a way to abstract to avoid the CPU-specific #ifdef - maybe a function in MacroAssembler-$(ARCH)? @@ +868,5 @@ > + masm.cmpPtr(code, ImmWord(StaticStrings::UNIT_STATIC_LIMIT)); > + if (!bailoutIf(Assembler::AboveOrEqual, lir->snapshot())) > + return false; > + > +#ifdef JS_CPU_ARM Same here.

(In reply to David Anderson [:dvander] from comment #5) > Comment on attachment 591797 [details] [diff] [review] > Inline native calls. (Math.abs, Math.round, Math.floor) > > Review of attachment 591797 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/ion/IonBuilder.h > @@ +305,5 @@ > > bool jsop_setprop(JSAtom *atom); > > bool jsop_newarray(uint32 count); > > bool jsop_this(); > > > > + /* Optimizing */ > > nit: This comment should be elaborated or removed. > > ::: js/src/ion/MCallOptimize.cpp > @@ +54,5 @@ > > + // Copy PassArg arguments from ArgN to This. > > + for (int32 i = argc; i >= 0; i--) { > > + MPassArg *passArg = current->pop()->toPassArg(); > > + JS_ASSERT(passArg->useCount() == 0); > > + passArg->block()->discard(passArg); > > Here, you want: > passArg->replaceAllUsesWith(wrapped); > passArg->remove(passArg); > > and remove the call to the discard. remove() removes it from use chains, > whichis needed here since the PassArg could be captured in resume points. > > @@ +64,5 @@ > > + current->pop(); > > + // FIXME: Why is the function useCount != 0 ? > > + // MInstruction *fun = current->pop()->toInstruction(); > > + // JS_ASSERT(fun->useCount() == 0); > > + // fun->block()->discard(fun); > > (answered above) This do not apply because we don't have something to replace it. > ::: js/src/ion/MIR.h > @@ +988,5 @@ > > : public MVariadicInstruction, > > public CallPolicy > > { > > + public: > > + struct SpecializeInfo { > > This structure isn't used in this patch, and it feels a little awkward. If > it's used in a later patch, let's wait to see how it gets used there, and > avoid checking it in early. I removed it as it does not work as expected because I am unable to make call site moving due to MPrepareCall which plies an order graph for MIR instructions. > ::: js/src/ion/arm/CodeGenerator-arm.cpp > @@ +865,5 @@ > > + return false; > > + > > + // input =< 0 > > + masm.bind(&belowZero); > > + masm.subsd(input, ScratchFloatReg); > > subsd (and addsd above) are x86-specific right? you might want Marty to > review the arm parts. Thanks, I have to check on ARM …

Rebase & Apply nits.

CodeGenerator-arm.cpp: In member function ‘virtual bool js::ion::CodeGeneratorARM::visitRound(js::ion::LRound*)’: CodeGenerator-arm.cpp:831:50: error: no matching function for call to ‘js::ion::MacroAssembler::ma_vsub(const js::ion::FloatRegister&, const js::ion::FloatRegister&)’ ../ion/arm/MacroAssembler-arm.h:285:10: note: candidate is: void js::ion::MacroAssemblerARM::ma_vsub(js::ion::FloatRegister, js::ion::FloatRegister, js::ion::FloatRegister) CodeGenerator-arm.cpp:834:27: error: ‘VFP_BelowOrEqual’ is not a member of ‘js::ion::Assembler’

(In reply to David Anderson [:dvander] from comment #6) > Comment on attachment 591798 [details] [diff] [review] > Inline native calls [2] (String.charAt, String.charCodeAt, > String.fromCharCode) > > Review of attachment 591798 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good overall, just a few things that should be addressed first: > > ::: js/src/ion/CodeGenerator.cpp > @@ +828,5 @@ > > + masm.testl(output, Imm32(JSString::ROPE_BIT)); > > + masm.j(Assembler::NotEqual, ool->entry()); > > + masm.bind(ool->rejoin()); > > + > > + masm.lshiftPtr(Imm32(1), index); > > Somewhere in here you need a guard for the string length. The guard has to > be a bailout, since charCodeAt(>= .length) returns... NaN :) > > Ideally though, you could emit an MBoundsCheck(index, MStringLength) or > something. This is exactly what I did in MCallOptimize.

Move load16 to MacroAssembler. Use BaseIndex for load16 and loadPtr.

Comment on attachment 595080 [details] [diff] [review] Inline native calls (String.charAt, String.charCodeAt, String.fromCharCode) Review of attachment 595080 [details] [diff] [review]: ----------------------------------------------------------------- Nice work. Just some small nits and suggestions: ::: js/src/ion/CodeGenerator.cpp @@ +56,5 @@ > : CodeGeneratorSpecific(gen, graph) > { > } > > +class OutOfLineEnsureLinear : public OutOfLineCodeBase<CodeGenerator> I don't know if it works, but one thing we could try is moving these one-off OutOfLine classes into function scope. @@ +93,5 @@ > + pushArg(str); > + if (!callVM(ensureLinearInfo, lir)) > + return false; > + > + restoreLive(lir); You need a masm.jump(ool->rejoin()) here, otherwise bad stuff will happen - might be tricky but you could try writing a test that this path runs (str + str should give you a non-linear string). @@ +1019,5 @@ > + > + Address lengthAndFlagsAddr(str, JSString::offsetOfLengthAndFlags()); > + masm.loadPtr(lengthAndFlagsAddr, output); > + > + masm.testl(output, Imm32(JSString::ROPE_BIT)); This is right, but I think it's better to test LINEAR_MASK. @@ +1036,5 @@ > + Register code = ToRegister(lir->code()); > + Register output = ToRegister(lir->output()); > + > + masm.cmpPtr(code, ImmWord(StaticStrings::UNIT_STATIC_LIMIT)); > + if (!bailoutIf(Assembler::AboveOrEqual, lir->snapshot())) Just a note, this is a place where we could bailout but not have any feedback during recompilation, so it's a potential (albeit, seemingly unlikely) performance fault. You might want to use an out-of-line path here instead. @@ +1039,5 @@ > + masm.cmpPtr(code, ImmWord(StaticStrings::UNIT_STATIC_LIMIT)); > + if (!bailoutIf(Assembler::AboveOrEqual, lir->snapshot())) > + return false; > + > + Scale scale = (sizeof(JSAtom *) == 4) ? TimesFour : TimesEight; You can just use ScalePointer. ::: js/src/ion/LIR-Common.h @@ +808,5 @@ > + return this->getDef(0); > + } > +}; > + > +// Adds two string, returning a string. These comments seem out of date. ::: js/src/ion/MCallOptimize.cpp @@ +150,5 @@ > + native == js::str_fromCharCode) { > + > + if (native == js_str_charCodeAt) { > + if (returnType != MIRType_Int32 || thisType != MIRType_String || > + arg1Type != MIRType_Int32) nit: Braces needed here (multi-line if expression) @@ +157,5 @@ > + > + if (native == js_str_charAt) { > + if (returnType != MIRType_String || thisType != MIRType_String || > + arg1Type != MIRType_Int32) > + return false; ditto @@ +182,5 @@ > + indexOrCode = ins; > + } > + if (native == js::str_fromCharCode || native == js_str_charAt) { > + ins = new MFromCharCode(indexOrCode); > + current->add(ins); Cool - I like how this chains together. ::: js/src/ion/arm/MacroAssembler-arm.cpp @@ +1156,5 @@ > +MacroAssemblerARMCompat::load16(const Address &src, const Register &dest) > +{ > + ma_ldrh(EDtrAddr(src.base), EDtrOffImm(src.offset)), dest); > +} > +void nit: Newline above void (same below) ::: js/src/ion/shared/Assembler-shared.h @@ +62,5 @@ > > explicit Imm32(int32_t value) : value(value) > { } > + > + static inline Imm32 shiftOf(enum Scale s) { style nit: ShiftOf (camelcase for most statics) @@ +75,5 @@ > + return Imm32(3); > + }; > + } > + > + static inline Imm32 factorOf(enum Scale s) { ditto ::: js/src/vm/String.h @@ +297,5 @@ > inline JSFlatString *ensureFlat(JSContext *cx); > inline JSFixedString *ensureFixed(JSContext *cx); > > + static > + bool ensureLinear(JSContext *cx, JSString *str) { style nit: static and bool should be on the same line inside class definitions

https://hg.mozilla.org/projects/ionmonkey/rev/e30f6ac05651 https://hg.mozilla.org/projects/ionmonkey/rev/6dd34eec6fbe