693724 - IonMonkey: Support monomorphic function calls

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

This goes with the function call work. We want to use Type Inference to guide in baking in the target of a call. The mechanics of this decision should be closely related to call inlining. We want to decide, either at the CALL(ARG,LOCAL,etc) or the NEW/CALL itself, if we can bake in the target of a call and what sort of guards it needs.

It seems like it makes the most sense to make this decision at the JSOP_CALL/NEW itself.

Comment 1

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #0)
> This goes with the function call work. We want to use Type Inference to
> guide in baking in the target of a call.

The same problem appeared when I tried to optimize JSOP_INITELEM.  Without type information, this instruction use the default "stub"-call extracted from the interpreter.  With type information matching a series of MIR types, the function call can be replaced by an optimized version.

I think it could be better to delegate this phase on the MIR.  Thus we can use the same system for the two problems.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

> I think it could be better to delegate this phase on the MIR.  Thus we can
> use the same system for the two problems.

Could you explain what you mean by this? The TypeOracle API is effectively one system, but the way it is queried and used to build MIR is different for each operation.

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #2)
> > I think it could be better to delegate this phase on the MIR.  Thus we can
> > use the same system for the two problems.
> 
> Could you explain what you mean by this? The TypeOracle API is effectively
> one system, but the way it is queried and used to build MIR is different for
> each operation.

I was thinking about TypePolicy and let the MIR class inherit from the type policy and let it select what is the best series of types which can match possible implementations with some preferences.  The selection of the type set an integer which identify the configuration.  Thus the LIR generation van be specialized by looking at the config number.

refs:
JSOP_INITELEM: Bug 691340
draft version of TypeSetPolicy: Attachment 564251 [details] [diff] (select from type)
usage of draft: Attachment 564254 [details] [diff] (LIR choice based on type)

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

The model we have so far is to make specialization decisions while building MIR, and to emit specialized MIR. It takes pressure off the rest of the compilation pipeline and means we don't have to pessimistically capture as many MResumePoints. It also makes inlining of calls and polymorphic sites a little easier.

But it's still an unknown, so let's look at these things on a case-by-case basis. In some cases it might be easier to have generic MIR where the real specialization happens in LIR.

In the meantime, the way we're using TypeSets right now is confusing. We should remove TypeSet from MInstruction. The only instruction that needs a TypeSet is MTypeBarrier (coming in the GETGNAME patch). For everything else, we make type decisions as soon as we push MIR onto a block, and the type analysis phase is for deciding representations and coercion.

Comment 5

[Sean Stangl [:sstangl]]

Attachment: Skip unnecessary checks when target function is known.

Eliminates certain checks in visitCallGeneric() when the callee is known:

- Function class check
- Native check (handled instead by LCallNative)
- Insufficient arguments check (missing arguments added in IonBuilder)

I think this is about the best we can do with monomorphic calls. The major win is already present as part of JSOP_CALLGNAME resulting in an MConstant.

Noticeably faster on v8, although the differece is only about 40ms out of ~6800 (outside error range). Doesn't affect SunSpider in a meaningful way.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 595528 [details] [diff] [review]
Skip unnecessary checks when target function is known.

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

::: js/src/ion/CodeGenerator.cpp
@@ +614,2 @@
>  #else
> +        masm.load16(Address(calleereg, offsetof(JSFunction, nargs)), nargsreg);

Pre-existing nit: Can we get rid of this #ifdef?

Comment 7

[Sean Stangl [:sstangl]]

Attachment: partial load16() support for ARM.

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Sean Stangl from comment #7)
> Created attachment 595587 [details] [diff] [review]
> partial load16() support for ARM.

Otherwise you can use load16 provided in https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=718853&attachment=595080

Comment 9

[Sean Stangl [:sstangl]]

(In reply to Nicolas B. Pierron [:pierron] from comment #8)
> Otherwise you can use load16 provided in
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=718853&attachment=595080

That's identical to the one in this patch.

Comment 10

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 595587 [details] [diff] [review]
partial load16() support for ARM.

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

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +1155,5 @@
>  void
> +MacroAssemblerARMCompat::load16(const Address &address, const Register &dest)
> +{
> +    ma_ldrh(EDtrAddr(address.base, EDtrOffImm(address.offset)), dest);
> +}

You should probably use dataTransferN.

MacroAssemblerARM::ma_dataTransferN(LoadStore ls, int size, bool IsSigned,
                          Register rn, Imm32 offset, Register rt,
                          Index mode, Assembler::Condition cc)

While it does not handle any more cases than your code, it will get them automatically when I add it for other sized loads.

Comment 11

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/projects/ionmonkey/rev/a32462495d86