Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add inlining support for asm.js/wasm #3681

Merged
merged 4 commits into from
Sep 14, 2017

Conversation

MikeHolman
Copy link
Contributor

@MikeHolman MikeHolman commented Sep 7, 2017

Add support for profiling in asm.js, and profile direct internal calls.

Use temp registers rather than hard coded registers for intermediate bytecode values, so backend correctly understands that these are temps and not locals. (Also allowed me to remove the I_Conv_* opcodes.)

Tell inliner to try to inline candidate asm.js/wasm functions. Required some special handling for functions with no parameters and non-var arguments, and skipping function object bailout check.

Simplify how we emit calls in asm.js bytecode generator to evaluate arguments before emitting argouts. Prior code required complicated tracking of call depth during bytecode gen, which led to complicated call nesting (and subsequently made inlining more complicated).

Track calls for asm.js/wasm during globopt, and change inline argument tracking to use inlinee argout size rather than count for frame size calculation. (This will be needed for stackwalking, which we punted for asm.js in general... but is something we should really get around to.)

For inlining heuristics, we are more aggressive at inlining than normal JS, for the reason that bytecode is already low level, so it will blow up in size less than JS bytecode will.

Preliminary perf indicates overall improvements of about 4.5% in Unity and 1% in Jetstream

Closes #2794


This change is Reviewable

case Js::OpCode::LdAsmJsEnv:
if (instr->m_func == inlinee)
{
instr->SetSrc1(funcObjOpnd);
Copy link
Contributor

@rajatd rajatd Sep 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this never be an AddrOpnd in asmjs? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, it's always a RegOpnd


In reply to: 138230447 [](ancestors = 138230447)

@rajatd
Copy link
Contributor

rajatd commented Sep 12, 2017

    instr->m_func = this->func;

why not do this for asmjs functions?


Refers to: lib/Backend/LinearScan.cpp:2113 in 7baad00. [](commit_id = 7baad00, deletion_comment = False)

@rajatd
Copy link
Contributor

rajatd commented Sep 12, 2017

Backend changes look good to me

@MikeHolman
Copy link
Contributor Author

    instr->m_func = this->func;

i think you're right this one i don't need. the reason for other one is we can introduce call during lowerer in an inlinee which was leaf, where the parent was also leaf, and only the inner func is updated. this leads to problems later where we end up encoding a call from a leaf func.
now though, i'm thinking probably better for LowerCall to do SetHasCallsOnSelfAndParents instead of SetHasCalls


In reply to: 328701050 [](ancestors = 328701050)


Refers to: lib/Backend/LinearScan.cpp:2113 in 7baad00. [](commit_id = 7baad00, deletion_comment = False)

@MikeHolman MikeHolman force-pushed the wasminline branch 6 times, most recently from b0b06fc to 3eca75e Compare September 12, 2017 23:26
@Cellule
Copy link
Contributor

Cellule commented Sep 13, 2017

Reviewed 67 of 74 files at r1, 9 of 10 files at r2.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.


a discussion (no related file):
One thing I noticed, is that right now we don't detect that the various LdAsmJsEnv in the inlined function are not considered to be the same as the root function (if they come from the same instance aka not a call_import).
But this sounds like an optimization that we should do in a separate change, just wanted to toss it out there.


a discussion (no related file):
Looks like the flag -trace:wasminout is broken with this change (ie: causes assert and/or AV)


a discussion (no related file):
I found a functional bug.
We can't allow to inline imported function because we share the code for multiple instances.
for instance the following test is broken

const buf = WebAssembly.wabt.convertWast2Wasm(`
(module
    (type $t1 (func (result i32)))
    (type $t2 (func (param i32) (result i32)))
    (import "test" "foo" (func $foo (type $t1)))
    (import "test" "print" (func $print (type $t2)))
    (func $f2 (export "a") (type $t2) (local f32)
      (call $print (call $foo))
    )
    (func (export "f2") (type $t1)
      (i32.const 2)
    )
)`);
const module = new WebAssembly.Module(buf);
const {exports: {a: a1, f2}} = new WebAssembly.Instance(module, {test: {foo: () => 1, print}});
const {exports: {a: a2}} = new WebAssembly.Instance(module, {test: {foo: f2, print}});
a1();
a1();
a2();

Without inlining, it prints 1 1 2 and with inling it prints 1 1 1.
Right now we can't inline import thunks because it might change depending on the instance


lib/Backend/BackendOpCodeAttrAsmJs.cpp, line 17 at r2 (raw file):

        OpHasProfiled = 1 << 2,
        OpProfiled = 1 << 3,
        OpByteCodeOnly = 1 << 4

We seem to only set the OpByteCodeOnly on Profiled opcode, but don't actually use it.
Can we remove this ?


lib/Backend/Sym.h, line 179 at r2 (raw file):

    void            SetIsFromByteCodeConstantTable() { this->m_isFromByteCodeConstantTable = true; }
    Js::ArgSlot     GetArgSlotNum() const { Assert(HasArgSlotNum()); return m_slotNum; }
    void            SetArgSlotNum(Js::ArgSlot newNum) { m_slotNum = newNum; }

I can't find any references to this function. Is it still needed ?


lib/Runtime/Language/DynamicProfileInfo.cpp, line 417 at r2 (raw file):

        if (!callerBody || !callerBody->GetIsAsmjsMode() || !calleeBody || !calleeBody->GetIsAsmjsMode())
        {
            AssertMsg(UNREACHED, "Call to RecordAsmJsCallSiteInfo without 2 wasm function body");

nit: asm.js/wasm function body


lib/WasmReader/WasmByteCodeGenerator.cpp, line 966 at r2 (raw file):

        WasmFunctionInfo* calleeInfo = m_module->GetWasmFunctionInfo(funcNum);
        calleeSignature = calleeInfo->GetSignature();
        if (!isImportCall)

change check for GetReader()->m_currentNode.call.funcType == FunctionIndexTypes::Function


lib/WasmReader/WasmByteCodeGenerator.cpp, line 1136 at r2 (raw file):

                throw WasmCompilationException(_u("Unknown call return type %u"), retInfo.type);
            }
            if (retInfo.type != WasmTypes::Void)

unnessary check since we have the same check above.


test/AsmJs/rlexe.xml, line 613 at r2 (raw file):

      <files>cseBug.js</files>
      <baseline>cseBug.baseline</baseline>
      <compile-flags>-testtrace:asmjs -off:deferparse</compile-flags>

Why was this needed ?


test/wasm/inlining.js, line 1 at r2 (raw file):

//-------------------------------------------------------------------------------------------------------

You didn't add this file to wasm/rlexe.xml


Comments from Reviewable

@Cellule
Copy link
Contributor

Cellule commented Sep 13, 2017

Reviewed 5 of 5 files at r3.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.


Comments from Reviewable

@MikeHolman
Copy link
Contributor Author

Review status: 76 of 77 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.


a discussion (no related file):

Previously, Cellule (Michael Ferris) wrote…

Looks like the flag -trace:wasminout is broken with this change (ie: causes assert and/or AV)

Disabled inlining/profiling when this is enabled and seems to work now


a discussion (no related file):

Previously, Cellule (Michael Ferris) wrote…

One thing I noticed, is that right now we don't detect that the various LdAsmJsEnv in the inlined function are not considered to be the same as the root function (if they come from the same instance aka not a call_import).
But this sounds like an optimization that we should do in a separate change, just wanted to toss it out there.

Yeah, I thought about this, but seems like something we can optimize later


lib/Backend/BackendOpCodeAttrAsmJs.cpp, line 17 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

We seem to only set the OpByteCodeOnly on Profiled opcode, but don't actually use it.
Can we remove this ?

Done.


lib/Backend/Sym.h, line 179 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

I can't find any references to this function. Is it still needed ?

Done.


lib/Runtime/Language/DynamicProfileInfo.cpp, line 417 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

nit: asm.js/wasm function body

Done.


lib/WasmReader/WasmByteCodeGenerator.cpp, line 966 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

change check for GetReader()->m_currentNode.call.funcType == FunctionIndexTypes::Function

Done.


lib/WasmReader/WasmByteCodeGenerator.cpp, line 1136 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

unnessary check since we have the same check above.

Done.


test/AsmJs/rlexe.xml, line 613 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

Why was this needed ?

something changed now that we are profiling causing different deferral between interpreter/dynapogo so trace was printing in different spot


test/wasm/inlining.js, line 1 at r2 (raw file):

Previously, Cellule (Michael Ferris) wrote…

You didn't add this file to wasm/rlexe.xml

Done.


Comments from Reviewable

@Cellule
Copy link
Contributor

Cellule commented Sep 13, 2017

:lgtm:


Reviewed 8 of 8 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@chakrabot chakrabot merged commit 75f5e8f into chakra-core:master Sep 14, 2017
chakrabot pushed a commit that referenced this pull request Sep 14, 2017
Merge pull request #3681 from MikeHolman:wasminline

Add support for profiling in asm.js, and profile direct internal calls.

Use temp registers rather than hard coded registers for intermediate bytecode values, so backend correctly understands that these are temps and not locals. (Also allowed me to remove the I_Conv_* opcodes.)

Tell inliner to try to inline candidate asm.js/wasm functions. Required some special handling for functions with no parameters and non-var arguments, and skipping function object bailout check.

Simplify how we emit calls in asm.js bytecode generator to evaluate arguments before emitting argouts. Prior code required complicated tracking of call depth during bytecode gen, which led to complicated call nesting (and subsequently made inlining more complicated).

Track calls for asm.js/wasm during globopt, and change inline argument tracking to use inlinee argout size rather than count for frame size calculation. (This will be needed for stackwalking, which we punted for asm.js in general... but is something we should really get around to.)

For inlining heuristics, we are more aggressive at inlining than normal JS, for the reason that bytecode is already low level, so it will blow up in size less than JS bytecode will.

Preliminary perf indicates overall improvements of about 4.5% in Unity and 1% in Jetstream

Closes #2794

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/microsoft/chakracore/3681)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants