Fix Optimisation of arguments[-n]

Author: rhuanjl

lib/Backend/GlobOpt.cpp

@@ -1586,7 +1586,7 @@ GlobOpt::OptArguments(IR::Instr *instr)
     }
 
     SymID id = 0;
-    
+
     switch(instr->m_opcode)
     {
     case Js::OpCode::LdElemI_A:
@@ -13529,16 +13529,19 @@ GlobOpt::OptStackArgLenAndConst(IR::Instr* instr, Value** src1Val)
                 {
                     int argIndex = indirOpndSrc1->GetOffset() + 1;
                     IR::Instr* defInstr = nullptr;
-                    IR::Instr* inlineeStart = instr->m_func->GetInlineeStart();
-                    inlineeStart->IterateArgInstrs([&](IR::Instr* argInstr) {
-                        StackSym *argSym = argInstr->GetDst()->AsSymOpnd()->m_sym->AsStackSym();
-                        if (argSym->GetArgSlotNum() - 1 == argIndex)
-                        {
-                            defInstr = argInstr;
-                            return true;
-                        }
-                        return false;
-                    });
+                    if (argIndex > 0)
+                    {
+                        IR::Instr* inlineeStart = instr->m_func->GetInlineeStart();
+                        inlineeStart->IterateArgInstrs([&](IR::Instr* argInstr) {
+                            StackSym *argSym = argInstr->GetDst()->AsSymOpnd()->m_sym->AsStackSym();
+                            if (argSym->GetArgSlotNum() - 1 == argIndex)
+                            {
+                                defInstr = argInstr;
+                                return true;
+                            }
+                            return false;
+                        });
+                    }
 
                     Js::OpCode replacementOpcode;
                     if (instr->m_opcode == Js::OpCode::TypeofElem)

lib/Backend/Lower.cpp

@@ -1,5 +1,6 @@
 //-------------------------------------------------------------------------------------------------------
 // Copyright (C) Microsoft Corporation and contributors. All rights reserved.
+// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
 // Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
 //-------------------------------------------------------------------------------------------------------
 #include "Backend.h"
@@ -22154,10 +22155,14 @@ Lowerer::GenerateFastArgumentsLdElemI(IR::Instr* ldElem, IR::LabelInstr *labelFa
     //  ---GenerateLdValueFromCheckedIndexOpnd
     //  ---LoadInputParamCount
     //  CMP actualParamOpnd, valueOpnd //Compare between the actual count & the index count (say i in arguments[i])
-    //  JLE $labelCreateHeapArgs
+    //  JLE $labelReturnUndefined
     //  MOV dst, ebp [(valueOpnd + 5) *4]  // 5 for the stack layout
     //  JMP $fallthrough
     //
+    //labelReturnUndefined:
+    //  MOV dst, undefined
+    //  JMP $fallthrough
+    //
     //labelCreateHeapArgs:
     //  ---Bail out to create Heap Arguments object
 
@@ -22179,12 +22184,23 @@ Lowerer::GenerateFastArgumentsLdElemI(IR::Instr* ldElem, IR::LabelInstr *labelFa
 
     bool hasIntConstIndex = indirOpnd->TryGetIntConstIndexValue(true, &value, &isNotInt);
 
-    if (isNotInt || (isInlinee && hasIntConstIndex && value >= (ldElem->m_func->actualCount - 1)))
+    if (isNotInt)
     {
-        //Outside the range of actuals, skip
+        //Not an int disable optimisation and rejit
     }
-    else if (labelFallThru != nullptr && !(hasIntConstIndex && value < 0)) //if index is not a negative int constant
+    else if (hasIntConstIndex && (value < 0 || (isInlinee && value >= (ldElem->m_func->actualCount - 1))))
     {
+        // if the index is an int const outside the range then the value must be undefined
+        // this is ensured as GlobOpt::OptArguments disables the Arguments optimisation if the arguments object is modified
+        IR::Opnd *undef = LoadLibraryValueOpnd(ldElem, LibraryValue::ValueUndefined);
+        Lowerer::InsertMove(ldElem->GetDst(), undef, ldElem);
+        // JMP $done
+        InsertBranch(Js::OpCode::Br, labelFallThru, ldElem);
+        emittedFastPath = true;
+    }
+    else if (labelFallThru != nullptr)
+    {
+        IR::LabelInstr *labelReturnUndefined = IR::LabelInstr::New(Js::OpCode::Label, func, true);
         if (isInlinee)
         {
             actualParamOpnd = IR::IntConstOpnd::New(ldElem->m_func->actualCount - 1, TyInt32, func);
@@ -22203,7 +22219,7 @@ Lowerer::GenerateFastArgumentsLdElemI(IR::Instr* ldElem, IR::LabelInstr *labelFa
         }
         else
         {
-            //Load valueOpnd from the index
+            //Load valueOpnd from the index, note this operation includes a bail-out for non-integer indices
             valueOpnd =
                 m_lowererMD.LoadNonnegativeIndex(
                     indexOpnd,
@@ -22215,22 +22231,22 @@ Lowerer::GenerateFastArgumentsLdElemI(IR::Instr* ldElem, IR::LabelInstr *labelFa
                         true
 #endif
                         ),
-                    labelCreateHeapArgs,
-                    labelCreateHeapArgs,
+                    labelReturnUndefined,
+                    labelReturnUndefined,
                     ldElem);
         }
 
         if (isInlinee)
         {
             if (!hasIntConstIndex)
             {
-                //Runtime check if to make sure length is within the arguments.length range.
-                GenerateCheckForArgumentsLength(ldElem, labelCreateHeapArgs, valueOpnd, actualParamOpnd, Js::OpCode::BrGe_A);
+                //Runtime check to make sure length is within the arguments.length range.
+                GenerateCheckForArgumentsLength(ldElem, labelReturnUndefined, valueOpnd, actualParamOpnd, Js::OpCode::BrGe_A);
             }
         }
         else
         {
-            GenerateCheckForArgumentsLength(ldElem, labelCreateHeapArgs, actualParamOpnd, valueOpnd, Js::OpCode::BrLe_A);
+            GenerateCheckForArgumentsLength(ldElem, labelReturnUndefined, actualParamOpnd, valueOpnd, Js::OpCode::BrLe_A);
         }
 
         IR::Opnd *argIndirOpnd = nullptr;
@@ -22244,9 +22260,16 @@ Lowerer::GenerateFastArgumentsLdElemI(IR::Instr* ldElem, IR::LabelInstr *labelFa
         }
 
         Lowerer::InsertMove(ldElem->GetDst(), argIndirOpnd, ldElem);
+        // JMP $done
+        InsertBranch(Js::OpCode::Br, labelFallThru, ldElem);
 
+        // if load is outside of range at runtime return false
+        ldElem->InsertBefore(labelReturnUndefined);
+        IR::Opnd *undef = LoadLibraryValueOpnd(ldElem, LibraryValue::ValueUndefined);
+        Lowerer::InsertMove(ldElem->GetDst(), undef, ldElem);
         // JMP $done
         InsertBranch(Js::OpCode::Br, labelFallThru, ldElem);
+
         // $labelCreateHeapArgs:
         ldElem->InsertBefore(labelCreateHeapArgs);
         emittedFastPath = true;

test/Function/StackArgsWithFormals.baseline

@@ -19,7 +19,6 @@ StackArgFormals : test16 (22) :Removing Heap Arguments object creation in Lowere
 StackArgFormals : test16 (22) :Removing Scope object creation in Lowerer and replacing it with MOV NULL. 
 StackArgFormals : test16 (22) :Attaching the scope object with the heap arguments object in the bail out path. 
 StackArgFormals : test17 (23) :Removing Heap Arguments object creation in Lowerer. 
-StackArgFormals : test17 (23) :Attaching the scope object with the heap arguments object in the bail out path. 
 StackArgFormals : test19 (25) :Removing Heap Arguments object creation in Lowerer. 
 StackArgFormals : test20 (26) :Removing Heap Arguments object creation in Lowerer. 
 PASSED

test/Optimizer/StackArgumentsOptNegativeIndex.js

@@ -0,0 +1,99 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft Corporation and contributors. All rights reserved.
+// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+let fail = 0;
+
+// Test 1 inline negative index
+function func1() {
+    return func2(5);
+}
+
+function func2() {
+    return arguments[-1];
+}
+
+const obj = { "o" : func2 };
+
+for (i = 0; i < 500; i++) {
+    if(func1() != undefined) {
+        ++ fail;
+    }
+}
+
+// Test 2 edited negative index
+function func3() {
+    return func4(5);
+}
+
+function func4() {
+    arguments[-1] = 5;
+    return arguments[-1];
+}
+
+for (i = 0; i < 500; i++) {
+    if(func3() != 5) {
+        ++ fail;
+    }
+}
+
+// Test 3 edit the negative index after jitting
+function func5(count) {
+    if (count > 10) {
+        arguments[-2] = 5;
+    }
+    return arguments[-2];
+}
+
+for (let i = 0; i < 20; ++i) {
+    let val = func5(i);
+    if (i > 10 && val !== 5 || i <= 10 && val !== undefined) {
+        ++fail;
+    }
+}
+
+function func6(count) {
+    return func5(count);
+}
+
+for (let i = 0; i < 20; ++i) {
+    let val = func6(i);
+    if (i > 10 && val !== 5 || i <= 10 && val !== undefined) {
+        ++fail;
+    }
+}
+
+// Test 4 dynamic negative index
+function func7(count) {
+    let n = 0;
+    if (count > 10) {
+        n = -1;
+    }
+    if (count > 20) {
+        n = 1;
+    }
+    return arguments[n];
+}
+
+function func8(count) {
+    return func7(count);
+}
+
+for (let i = 0; i < 40; ++i) {
+    let val = func7(i);
+    if (i < 11 && val !== i || i > 10 && val !== undefined) {
+        ++fail;
+    }
+}
+
+for (let i = 0; i < 40; ++i) {
+    let val = func8(i);
+    if (i < 11 && val !== i || i > 10 && val !== undefined) {
+        ++fail;
+    }
+}
+
+
+print (fail > 0 ? "fail" : "pass");

test/Optimizer/rlexe.xml

@@ -1641,4 +1641,9 @@
       <tags>exclude_nonative</tags>
     </default>
   </test>
+  <test>
+    <default>
+      <files>StackArgumentsOptNegativeIndex.js</files>
+    </default>
+  </test>
 </regress-exe>

test/stackfunc/funcexpr_2.js

@@ -1,5 +1,6 @@
 //-------------------------------------------------------------------------------------------------------
-// Copyright (C) Microsoft. All rights reserved.
+// Copyright (C) Microsoft Corporation and contributors. All rights reserved.
+// Copyright (c) 2021 ChakraCore Project Contributors. All rights reserved.
 // Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
 //-------------------------------------------------------------------------------------------------------
 
@@ -30,7 +31,7 @@ glo();
       function v2() {
       }
       argMath5 = eval;
-    })(arguments[0]);
+    })(arguments[0.5]);
     prop1 = argMath5;
   }
   v0();