Last Comment Bug 718853 - IonMonkey: Inline code for calls to built-in functions (Math.abs etc, like JM+TI)
: IonMonkey: Inline code for calls to built-in functions (Math.abs etc, like JM...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 715511
Blocks: 684381
  Show dependency treegraph
 
Reported: 2012-01-17 15:24 PST by Nicolas B. Pierron [:nbp]
Modified: 2012-02-21 14:56 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Replace native calls by optimized versions. (29.57 KB, patch)
2012-01-19 16:42 PST, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Inline native calls. (Math.abs, Math.round, Math.floor) (40.59 KB, patch)
2012-01-26 07:43 PST, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review
Inline native calls [2] (String.charAt, String.charCodeAt, String.fromCharCode) (26.78 KB, patch)
2012-01-26 07:44 PST, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Inline native calls. (Math.abs, Math.round, Math.floor) (37.15 KB, patch)
2012-02-03 15:18 PST, Nicolas B. Pierron [:nbp]
nicolas.b.pierron: review+
Details | Diff | Splinter Review
Inline native calls (String.charAt, String.charCodeAt, String.fromCharCode) (31.20 KB, patch)
2012-02-07 10:22 PST, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2012-01-17 15:24:19 PST
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.
Comment 1 Jan de Mooij [:jandem] 2012-01-18 01:53:35 PST
Cool, we really need this for Kraken gaussian-blur. IIRC, inlining Math.abs made JM+TI > 5x faster on that test.
Comment 2 Nicolas B. Pierron [:nbp] 2012-01-19 16:42:41 PST
Created attachment 590044 [details] [diff] [review]
Replace native calls by optimized versions.

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 3 Nicolas B. Pierron [:nbp] 2012-01-26 07:43:20 PST
Created attachment 591797 [details] [diff] [review]
Inline native calls. (Math.abs, Math.round, Math.floor)
Comment 4 Nicolas B. Pierron [:nbp] 2012-01-26 07:44:36 PST
Created attachment 591798 [details] [diff] [review]
Inline native calls [2] (String.charAt, String.charCodeAt, String.fromCharCode)
Comment 5 David Anderson [:dvander] 2012-01-27 17:30:28 PST
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 6 David Anderson [:dvander] 2012-01-27 17:46:05 PST
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.
Comment 7 Nicolas B. Pierron [:nbp] 2012-02-02 11:27:16 PST
(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 …
Comment 8 Nicolas B. Pierron [:nbp] 2012-02-03 15:18:13 PST
Created attachment 594323 [details] [diff] [review]
Inline native calls. (Math.abs, Math.round, Math.floor)

Rebase & Apply nits.
Comment 9 David Anderson [:dvander] 2012-02-03 18:45:36 PST
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’
Comment 10 Nicolas B. Pierron [:nbp] 2012-02-07 09:30:12 PST
(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.
Comment 11 Nicolas B. Pierron [:nbp] 2012-02-07 10:22:25 PST
Created attachment 595080 [details] [diff] [review]
Inline native calls (String.charAt, String.charCodeAt, String.fromCharCode)

Move load16 to MacroAssembler.
Use BaseIndex for load16 and loadPtr.
Comment 12 David Anderson [:dvander] 2012-02-16 18:35:45 PST
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

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