[MERGE #5654 @sigatrev] fix bailouts from inlined callback.call

Merge pull request #5654 from sigatrev:callbackFuncOpnd
chakra-core · Sep 10, 2018 · b45b70b · b45b70b
2 parents a3737b0 + 00184d7
commit b45b70b
Show file tree

Hide file tree

Showing 6 changed files with 64 additions and 13 deletions.
diff --git a/lib/Backend/Inline.cpp b/lib/Backend/Inline.cpp
@@ -1010,6 +1010,13 @@ Inline::InlinePolymorphicFunctionUsingFixedMethods(IR::Instr *callInstr, const F
     return instrNext;
 }
 
+IR::RegOpnd * Inline::GetCallbackFunctionOpnd(IR::Instr * callInstr)
+{
+    IR::Instr * callApplyLdInstr = callInstr->GetSrc1()->GetStackSym()->GetInstrDef();
+    IR::Instr * targetDefInstr = callApplyLdInstr->GetSrc1()->AsPropertySymOpnd()->GetObjectSym()->GetInstrDef();
+    return targetDefInstr->GetDst()->AsRegOpnd();
+}
+
 IR::Instr * Inline::TryGetCallbackDefInstrForCallInstr(IR::Instr * callInstr)
 {
     // Try to find a function argument that could be inlined.
@@ -2891,7 +2898,12 @@ bool Inline::InlineApplyScriptTarget(IR::Instr *callInstr, const FunctionJITTime
     StackSym* originalCallTargetStackSym = callInstr->GetSrc1()->GetStackSym();
     bool originalCallTargetOpndIsJITOpt = callInstr->GetSrc1()->GetIsJITOptimizedReg();
     bool safeThis = false;
-    if (!targetIsCallback && !TryGetFixedMethodsForBuiltInAndTarget(callInstr, inlinerData, inlineeData, applyFuncInfo, applyLdInstr, applyTargetLdInstr, safeThis, /*isApplyTarget*/ true))
+
+    if (targetIsCallback)
+    {
+        callInstr->ReplaceSrc1(GetCallbackFunctionOpnd(callInstr));
+    }
+    else if (!TryGetFixedMethodsForBuiltInAndTarget(callInstr, inlinerData, inlineeData, applyFuncInfo, applyLdInstr, applyTargetLdInstr, safeThis, /*isApplyTarget*/ true))
     {
         return false;
     }
@@ -3216,7 +3228,12 @@ Inline::InlineCallTarget(IR::Instr *callInstr, const FunctionJITTimeInfo* inline
     StackSym* originalCallTargetStackSym = callInstr->GetSrc1()->GetStackSym();
     bool originalCallTargetOpndIsJITOpt = callInstr->GetSrc1()->GetIsJITOptimizedReg();
     bool safeThis = false;
-    if (!targetIsCallback && !TryGetFixedMethodsForBuiltInAndTarget(callInstr, inlinerData, inlineeData, callFuncInfo, callLdInstr, callTargetLdInstr, safeThis, /*isApplyTarget*/ false))
+
+    if (targetIsCallback)
+    {
+        callInstr->ReplaceSrc1(GetCallbackFunctionOpnd(callInstr));
+    }
+    else if (!TryGetFixedMethodsForBuiltInAndTarget(callInstr, inlinerData, inlineeData, callFuncInfo, callLdInstr, callTargetLdInstr, safeThis, /*isApplyTarget*/ false))
     {
         return false;
     }
@@ -4433,17 +4450,7 @@ Inline::PrepareInsertionPoint(IR::Instr *callInstr, const FunctionJITTimeInfo *f
     Assert(insertBeforeInstr->m_func == callInstr->m_func);
     IR::BailOutKind bailOutKind = IR::BailOutOnInlineFunction;
 
-    IR::RegOpnd * funcOpnd;
-    if (isCallbackCallApplyTarget)
-    {
-        IR::Instr * callApplyLdInstr = callInstr->GetSrc1()->GetStackSym()->GetInstrDef();
-        IR::Instr * targetDefInstr = callApplyLdInstr->GetSrc1()->AsPropertySymOpnd()->GetObjectSym()->GetInstrDef();
-        funcOpnd = targetDefInstr->GetDst()->AsRegOpnd();
-    }
-    else
-    {
-        funcOpnd = callInstr->GetSrc1()->AsRegOpnd();
-    }
+    IR::RegOpnd * funcOpnd = callInstr->GetSrc1()->AsRegOpnd();
 
     // FunctionBody check is the primary bailout instruction, create it first
     IR::BailOutInstr* primaryBailOutInstr = IR::BailOutInstr::New(Js::OpCode::BailOnNotEqual, bailOutKind, insertBeforeInstr, callInstr->m_func);

diff --git a/lib/Backend/Inline.h b/lib/Backend/Inline.h
@@ -95,6 +95,8 @@ class Inline
         const bool isInlined, const bool doneFixedMethodFld, IR::Instr** createObjInstrOut, IR::Instr** callCtorInstrOut) const;
     IR::Instr * InlinePolymorphicFunction(IR::Instr *callInstr, const FunctionJITTimeInfo * inlinerData, const StackSym *symCallerThis, const Js::ProfileId profileId, bool* pIsInlined, uint recursiveInlineDepth, bool triedUsingFixedMethods = false);
     IR::Instr * InlinePolymorphicFunctionUsingFixedMethods(IR::Instr *callInstr, const FunctionJITTimeInfo * inlinerData, const StackSym *symCallerThis, const Js::ProfileId profileId, IR::PropertySymOpnd* methodValueOpnd, bool* pIsInlined, uint recursiveInlineDepth);
+
+    IR::RegOpnd * GetCallbackFunctionOpnd(IR::Instr * callInstr);
     IR::Instr * TryGetCallbackDefInstr(StackSym * callbackSym);
     IR::Instr * TryGetCallbackDefInstrForCallInstr(IR::Instr * callInstr);
     IR::Instr * TryGetCallbackDefInstrForCallApplyTarget(IR::Instr * callApplyLdInstr);

diff --git a/test/inlining/InlineCallbackCallBailout.baseline b/test/inlining/InlineCallbackCallBailout.baseline
@@ -0,0 +1,5 @@
+INLINING : Found callback def instr for call/apply target callback at	CallSite: 0	Caller: DispatchCallWithThis ( (#1.2), #3)
+INLINING CALLBACK : Inlining callback for call/apply target : 	BailOut ( (#1.1), #2)
+BailOut: function BailOut, Opcode: BoundCheck Kind: BailOutOnNotNativeArray
+BailOut: function DispatchCallWithThis, Opcode: InlineeEnd Kind: BailOutOnNotNativeArray
+BailOut: function DispatchBailout, Opcode: InlineeEnd Kind: BailOutOnNotNativeArray
diff --git a/test/inlining/InlineCallbackCallBailout.js b/test/inlining/InlineCallbackCallBailout.js
@@ -0,0 +1,24 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+// test bailout from inlined callback.call
+function BailOut(ary)
+{
+    ary[0] = 1;
+}
+
+function DispatchCallWithThis(callback, arg)
+{
+     callback.call(this, arg);
+}
+
+function DispatchBailout(arg)
+{
+    DispatchCallWithThis(BailOut, arg);
+}
+
+DispatchBailout([1]);
+DispatchBailout([1]);
+DispatchBailout([1.1]);
diff --git a/test/inlining/InlineCallbacks.js b/test/inlining/InlineCallbacks.js
@@ -3,6 +3,7 @@
 // Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
 //-------------------------------------------------------------------------------------------------------
 
+// Test inlining of callback.
 function Dispatch(f) { f(); }
 function Foo() { WScript.Echo("foo"); }
 function DispatchFoo() { Dispatch(Foo); }
@@ -17,13 +18,15 @@ DispatchFoo();
 DispatchFoo();
 DispatchFoo();
 
+// Test inlining of a callback function with a callback.
 function Bar() { WScript.Echo("bar"); }
 function DispatchBar() { Dispatch(Bar); }
 function NestedDispatch() { Dispatch(DispatchBar) };
 NestedDispatch();
 NestedDispatch();
 NestedDispatch();
 
+// Test inlining of callback with argument
 function Dispatch2(f, arg) { f(arg); }
 function Blah(arg) { WScript.Echo(arg); }
 function DispatchBlah(arg) { Dispatch2(Blah, arg) }
@@ -44,6 +47,7 @@ DispatchFooBar();
 DispatchFooBar();
 DispatchFooBar();
 
+// test inlining of callback.call
 function DispatchCall(callback, thisArg) { callback.call(thisArg); }
 function DispatchFooCall() { DispatchCall(Foo, {}); }
 DispatchCall(function(){});
@@ -52,6 +56,7 @@ DispatchFooCall();
 DispatchFooCall();
 DispatchFooCall();
 
+// test inlining of callback.apply
 function DispatchApply(callback, thisArg) { callback.apply(thisArg); }
 function DispatchBarApply() { DispatchApply(Bar, {}); }
 DispatchApply(function(){});

diff --git a/test/inlining/rlexe.xml b/test/inlining/rlexe.xml
@@ -304,6 +304,14 @@
       <tags>exclude_dynapogo,exclude_nonative,exclude_forceserialized,require_backend</tags>
     </default>
   </test>
+  <test>
+    <default>
+      <files>InlineCallbackCallBailout.js</files>
+      <compile-flags>-testtrace:InlineCallbacks -testtrace:Bailout</compile-flags>
+      <baseline>InlineCallbackCallBailout.baseline</baseline>
+      <tags>exclude_dynapogo,exclude_nonative,exclude_forceserialized,require_backend</tags>
+    </default>
+  </test>
   <test>
     <default>
       <files>recursiveCallbacks.js</files>