Bug 1533890: Move guardAndUpdateSpreadArgc logic inside the call op r=mgaudet
authorIain Ireland <iireland@mozilla.com>
Mon, 08 Apr 2019 15:28:17 +0000
changeset 468389 c1d550195b9fd52c9fd8a5086d062c032d53f7e1
parent 468388 f8a17aa391621c11ad8d3c9afd339e1c494a549d
child 468390 6d8643e52d18d6850bf29d531696935b840edfe1
push id35835
push useraciure@mozilla.com
push dateMon, 08 Apr 2019 19:00:29 +0000
treeherdermozilla-central@40456af7da1c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmgaudet
bugs1533890
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1533890: Move guardAndUpdateSpreadArgc logic inside the call op r=mgaudet The initial implementation of CacheIR spread calls added a guardAndUpdateSpreadArgc op, which had to be emitted just before the call. This patch moves the argc update inside the call op, where it belonged all along. In a few patches, fun_apply will also use this code. Differential Revision: https://phabricator.services.mozilla.com/D25869
js/src/jit/BaselineCacheIRCompiler.cpp
js/src/jit/CacheIR.cpp
js/src/jit/CacheIR.h
js/src/jit/IonCacheIRCompiler.cpp
--- a/js/src/jit/BaselineCacheIRCompiler.cpp
+++ b/js/src/jit/BaselineCacheIRCompiler.cpp
@@ -64,22 +64,24 @@ class MOZ_RAII BaselineCacheIRCompiler :
 
   MOZ_MUST_USE bool callTypeUpdateIC(Register obj, ValueOperand val,
                                      Register scratch,
                                      LiveGeneralRegisterSet saveRegs);
 
   MOZ_MUST_USE bool emitStoreSlotShared(bool isFixed);
   MOZ_MUST_USE bool emitAddAndStoreSlotShared(CacheOp op);
 
+  bool updateArgc(CallFlags flags, Register argcReg, Register scratch);
   void loadStackObject(ArgumentKind slot, CallFlags flags, size_t stackPushed,
                        Register argcReg, Register dest);
-  void pushCallArguments(Register argcReg, Register scratch, bool isJitCall,
-                         bool isConstructing);
+  void pushCallArguments(Register argcReg, Register scratch, Register scratch2,
+                         bool isJitCall, bool isConstructing);
   void pushSpreadCallArguments(Register argcReg, Register scratch,
-                               bool isJitCall, bool isConstructing);
+                               Register scratch2, bool isJitCall,
+                               bool isConstructing);
   void createThis(Register argcReg, Register calleeReg, Register scratch,
                   CallFlags flags);
   void updateReturnValue();
 
   enum class NativeCallType { Native, ClassHook };
   bool emitCallNativeShared(NativeCallType callType);
 
  public:
@@ -2414,69 +2416,86 @@ bool BaselineCacheIRCompiler::emitCallSt
 
   using Fn = bool (*)(JSContext*, HandleValue, HandleValue, MutableHandleValue);
   tailCallVM<Fn, DoConcatStringObject>(masm);
 
   return true;
 }
 
 
-bool BaselineCacheIRCompiler::emitGuardAndUpdateSpreadArgc() {
-  JitSpew(JitSpew_Codegen, __FUNCTION__);
-  Register argcReg = allocator.useRegister(masm, reader.int32OperandId());
-  AutoScratchRegister scratch(allocator, masm);
-  bool isConstructing = reader.readBool();
+// The value of argc entering the call IC is not always the value of
+// argc entering the callee. (For example, argc for a spread call IC
+// is always 1, but argc for the callee is the length of the array.)
+// In these cases, we update argc as part of the call op itself, to
+// avoid modifying input operands while it is still possible to fail a
+// guard. The code to update argc overlaps with some of the guard
+// code, so for the sake of efficiency we perform the final set of
+// guards here, just before updating argc. (In a perfect world, we
+// would have more registers and we would not need to worry about
+// modifying argc. In the real world, we have x86-32.)
+bool BaselineCacheIRCompiler::updateArgc(CallFlags flags, Register argcReg,
+                                         Register scratch) {
+  // Standard calls have no extra guards, and argc is already correct.
+  if (flags.getArgFormat() == CallFlags::Standard) {
+    return true;
+  }
 
   FailurePath* failure;
   if (!addFailurePath(&failure)) {
     return false;
   }
 
-  masm.unboxObject(Address(masm.getStackPointer(),
-                           isConstructing * sizeof(Value) + ICStackValueOffset),
-                   scratch);
-  masm.loadPtr(Address(scratch, NativeObject::offsetOfElements()), scratch);
-  masm.load32(Address(scratch, ObjectElements::offsetOfLength()), scratch);
-
-  // Limit actual argc to something reasonable (huge number of arguments can
-  // blow the stack limit).
-  static_assert(CacheIRCompiler::MAX_ARGS_SPREAD_LENGTH <= ARGS_LENGTH_MAX,
-                "maximum arguments length for optimized stub should be <= "
-                "ARGS_LENGTH_MAX");
-  masm.branch32(Assembler::Above, argcReg,
-                Imm32(CacheIRCompiler::MAX_ARGS_SPREAD_LENGTH),
-                failure->label());
-
-  // All guards have been passed. The call operation is no longer fallible.
-  // We update argcReg in-place, to avoid running out of registers on x86-32.
-  masm.move32(scratch, argcReg);
+  switch (flags.getArgFormat()) {
+    case CallFlags::Spread: {
+      // Find length of args array
+      BaselineFrameSlot slot(flags.isConstructing());
+      masm.unboxObject(allocator.addressOf(masm, slot), scratch);
+      masm.loadPtr(Address(scratch, NativeObject::offsetOfElements()), scratch);
+      masm.load32(Address(scratch, ObjectElements::offsetOfLength()), scratch);
+
+      // Limit actual argc to something reasonable to avoid blowing stack limit.
+      static_assert(CacheIRCompiler::MAX_ARGS_SPREAD_LENGTH <= ARGS_LENGTH_MAX,
+                    "maximum arguments length for optimized stub should be <= "
+                    "ARGS_LENGTH_MAX");
+      masm.branch32(Assembler::Above, scratch,
+                    Imm32(CacheIRCompiler::MAX_ARGS_SPREAD_LENGTH),
+                    failure->label());
+
+      // We're past the final guard. Overwrite argc with the new value.
+      masm.move32(scratch, argcReg);
+    } break;
+    default:
+      MOZ_CRASH("Unknown arg format");
+  }
+
   return true;
 }
 
 void BaselineCacheIRCompiler::pushCallArguments(Register argcReg,
                                                 Register scratch,
+                                                Register scratch2,
                                                 bool isJitCall,
                                                 bool isConstructing) {
   // The arguments to the call IC are pushed on the stack left-to-right.
   // Our calling conventions want them right-to-left in the callee, so
   // we duplicate them on the stack in reverse order.
   // |this| and callee are pushed last.
 
   // countReg contains the total number of arguments to copy.
   // In addition to the actual arguments, we have to copy the callee and |this|.
   // If we are constructing, we also have to copy newTarget.
   // We use a scratch register to avoid clobbering argc, which is an input reg.
   Register countReg = scratch;
   masm.move32(argcReg, countReg);
   masm.add32(Imm32(2 + isConstructing), countReg);
 
   // argPtr initially points to the last argument. Skip the stub frame.
-  AutoScratchRegister argPtr(allocator, masm);
+  Register argPtr = scratch2;
   Address argAddress(masm.getStackPointer(), STUB_FRAME_SIZE);
-  masm.computeEffectiveAddress(argAddress, argPtr.get());
+  masm.computeEffectiveAddress(argAddress, argPtr);
 
   // Align the stack such that the JitFrameLayout is aligned on the
   // JitStackAlignment.
   if (isJitCall) {
     masm.alignJitStackBasedOnNArgs(countReg);
   }
 
   // Push all values, starting at the last one.
@@ -2490,44 +2509,45 @@ void BaselineCacheIRCompiler::pushCallAr
     masm.sub32(Imm32(1), countReg);
     masm.jump(&loop);
   }
   masm.bind(&done);
 }
 
 void BaselineCacheIRCompiler::pushSpreadCallArguments(Register argcReg,
                                                       Register scratch,
+                                                      Register scratch2,
                                                       bool isJitCall,
                                                       bool isConstructing) {
   // Pull the array off the stack before aligning.
-  AutoScratchRegister startReg(allocator, masm);
+  Register startReg = scratch;
   masm.unboxObject(Address(masm.getStackPointer(),
                            (isConstructing * sizeof(Value)) + STUB_FRAME_SIZE),
                    startReg);
   masm.loadPtr(Address(startReg, NativeObject::offsetOfElements()), startReg);
 
   // Align the stack such that the JitFrameLayout is aligned on the
   // JitStackAlignment.
   if (isJitCall) {
     Register alignReg = argcReg;
     if (isConstructing) {
       // If we are constructing, we must take newTarget into account.
-      alignReg = scratch;
+      alignReg = scratch2;
       masm.computeEffectiveAddress(Address(argcReg, 1), alignReg);
     }
     masm.alignJitStackBasedOnNArgs(alignReg);
   }
 
   // Push newTarget, if necessary
   if (isConstructing) {
     masm.pushValue(Address(BaselineFrameReg, STUB_FRAME_SIZE));
   }
 
   // Push arguments: set up endReg to point to &array[argc]
-  Register endReg = scratch;
+  Register endReg = scratch2;
   BaseValueIndex endAddr(startReg, argcReg);
   masm.computeEffectiveAddress(endAddr, endReg);
 
   // Copying pre-decrements endReg by 8 until startReg is reached
   Label copyDone;
   Label copyStart;
   masm.bind(&copyStart);
   masm.branchPtr(Assembler::Equal, endReg, startReg, &copyDone);
@@ -2543,57 +2563,61 @@ void BaselineCacheIRCompiler::pushSpread
   masm.pushValue(
       Address(BaselineFrameReg,
               STUB_FRAME_SIZE + (2 + isConstructing) * sizeof(Value)));
 }
 
 bool BaselineCacheIRCompiler::emitCallNativeShared(NativeCallType callType) {
   AutoOutputRegister output(*this);
   AutoScratchRegisterMaybeOutput scratch(allocator, masm, output);
+  AutoScratchRegister scratch2(allocator, masm);
 
   Register calleeReg = allocator.useRegister(masm, reader.objOperandId());
   Register argcReg = allocator.useRegister(masm, reader.int32OperandId());
 
   CallFlags flags = reader.callFlags();
   bool isConstructing = flags.isConstructing();
   bool isSameRealm = flags.isSameRealm();
 
+  if (!updateArgc(flags, argcReg, scratch)) {
+    return false;
+  }
+
   allocator.discardStack(masm);
 
   // Push a stub frame so that we can perform a non-tail call.
   // Note that this leaves the return address in TailCallReg.
   AutoStubFrame stubFrame(*this);
   stubFrame.enter(masm, scratch);
 
   if (!isSameRealm) {
     masm.switchToObjectRealm(calleeReg, scratch);
   }
 
   switch (flags.getArgFormat()) {
     case CallFlags::Standard:
-      pushCallArguments(argcReg, scratch, /*isJitCall = */ false,
+      pushCallArguments(argcReg, scratch, scratch2, /*isJitCall =*/false,
                         isConstructing);
       break;
     case CallFlags::Spread:
-      pushSpreadCallArguments(argcReg, scratch, /*isJitCall = */ false,
+      pushSpreadCallArguments(argcReg, scratch, scratch2, /*isJitCall =*/false,
                               isConstructing);
       break;
     default:
       MOZ_CRASH("Invalid arg format");
   }
 
   // Native functions have the signature:
   //
   //    bool (*)(JSContext*, unsigned, Value* vp)
   //
   // Where vp[0] is space for callee/return value, vp[1] is |this|, and vp[2]
   // onward are the function arguments.
 
   // Initialize vp.
-  AutoScratchRegister scratch2(allocator, masm);
   masm.moveStackPtrTo(scratch2.get());
 
   // Construct a native exit frame.
   masm.push(argcReg);
 
   EmitBaselineCreateStubFrameDescriptor(masm, scratch, ExitFrameLayout::Size());
   masm.push(scratch);
   masm.push(ICTailCallReg);
@@ -2788,24 +2812,29 @@ void BaselineCacheIRCompiler::updateRetu
 #endif
   masm.bind(&skipThisReplace);
 }
 
 bool BaselineCacheIRCompiler::emitCallScriptedFunction() {
   JitSpew(JitSpew_Codegen, __FUNCTION__);
   AutoOutputRegister output(*this);
   AutoScratchRegisterMaybeOutput scratch(allocator, masm, output);
+  AutoScratchRegister scratch2(allocator, masm);
 
   Register calleeReg = allocator.useRegister(masm, reader.objOperandId());
   Register argcReg = allocator.useRegister(masm, reader.int32OperandId());
 
   CallFlags flags = reader.callFlags();
   bool isConstructing = flags.isConstructing();
   bool isSameRealm = flags.isSameRealm();
 
+  if (!updateArgc(flags, argcReg, scratch)) {
+    return false;
+  }
+
   allocator.discardStack(masm);
 
   // Push a stub frame so that we can perform a non-tail call.
   // Note that this leaves the return address in TailCallReg.
   AutoStubFrame stubFrame(*this);
   stubFrame.enter(masm, scratch);
 
   if (!isSameRealm) {
@@ -2813,37 +2842,37 @@ bool BaselineCacheIRCompiler::emitCallSc
   }
 
   if (isConstructing) {
     createThis(argcReg, calleeReg, scratch, flags);
   }
 
   switch (flags.getArgFormat()) {
     case CallFlags::Standard:
-      pushCallArguments(argcReg, scratch, /*isJitCall = */ true,
+      pushCallArguments(argcReg, scratch, scratch2, /*isJitCall = */ true,
                         isConstructing);
       break;
     case CallFlags::Spread:
-      pushSpreadCallArguments(argcReg, scratch, /*isJitCall = */ true,
+      pushSpreadCallArguments(argcReg, scratch, scratch2, /*isJitCall = */ true,
                               isConstructing);
       break;
     default:
       MOZ_CRASH("Invalid arg format");
   }
 
   // TODO: The callee is currently on top of the stack.  The old
   // implementation popped it at this point, but I'm not sure why,
   // because it is still in a register along both paths. For now we
   // just free that stack slot to make things line up. This should
   // probably be rewritten to avoid pushing callee at all if we don't
   // have to.
   masm.freeStack(sizeof(Value));
 
   // Load the start of the target JitCode.
-  AutoScratchRegister code(allocator, masm);
+  Register code = scratch2;
   masm.loadJitCodeRaw(calleeReg, code);
 
   EmitBaselineCreateStubFrameDescriptor(masm, scratch, JitFrameLayout::Size());
 
   // Note that we use Push, not push, so that callJit will align the stack
   // properly on ARM.
   masm.Push(argcReg);
   masm.PushCalleeToken(calleeReg, isConstructing);
@@ -2868,14 +2897,13 @@ bool BaselineCacheIRCompiler::emitCallSc
   // replace it with the |this| object passed in.
   if (isConstructing) {
     updateReturnValue();
   }
 
   stubFrame.leave(masm, true);
 
   if (!isSameRealm) {
-    // Use |code| as a scratch register.
-    masm.switchToBaselineFrameRealm(code);
+    masm.switchToBaselineFrameRealm(scratch2);
   }
 
   return true;
 }
--- a/js/src/jit/CacheIR.cpp
+++ b/js/src/jit/CacheIR.cpp
@@ -5087,21 +5087,16 @@ bool CallIRGenerator::tryAttachCallScrip
 
   // Ensure callee matches this stub's callee
   FieldOffset calleeOffset =
       writer.guardSpecificObject(calleeObjId, calleeFunc);
 
   // Guard against relazification
   writer.guardFunctionHasJitEntry(calleeObjId, isConstructing);
 
-  // Enforce limits on spread call length, and update argc.
-  if (isSpread) {
-    writer.guardAndUpdateSpreadArgc(argcId, isConstructing);
-  }
-
   writer.callScriptedFunction(calleeObjId, argcId, flags);
   writer.typeMonitorResult();
 
   if (templateObj) {
     writer.metaScriptedTemplateObject(templateObj, calleeOffset);
   }
 
   cacheIRStubKind_ = BaselineCacheIRStubKind::Monitored;
@@ -5231,21 +5226,16 @@ bool CallIRGenerator::tryAttachCallNativ
   ValOperandId calleeValId =
       writer.loadArgumentDynamicSlot(ArgumentKind::Callee, argcId, flags);
   ObjOperandId calleeObjId = writer.guardIsObject(calleeValId);
 
   // Ensure callee matches this stub's callee
   FieldOffset calleeOffset =
       writer.guardSpecificObject(calleeObjId, calleeFunc);
 
-  // Enforce limits on spread call length, and update argc.
-  if (isSpread) {
-    writer.guardAndUpdateSpreadArgc(argcId, isConstructing);
-  }
-
   writer.callNativeFunction(calleeObjId, argcId, op_, calleeFunc, flags);
   writer.typeMonitorResult();
 
   if (templateObj) {
     writer.metaNativeTemplateObject(templateObj, calleeOffset);
   }
 
   cacheIRStubKind_ = BaselineCacheIRStubKind::Monitored;
@@ -5306,21 +5296,16 @@ bool CallIRGenerator::tryAttachCallHook(
   ValOperandId calleeValId =
       writer.loadArgumentDynamicSlot(ArgumentKind::Callee, argcId, flags);
   ObjOperandId calleeObjId = writer.guardIsObject(calleeValId);
 
   // Ensure the callee's class matches the one in this stub.
   FieldOffset classOffset =
       writer.guardAnyClass(calleeObjId, calleeObj->getClass());
 
-  // Enforce limits on spread call length, and update argc.
-  if (isSpread) {
-    writer.guardAndUpdateSpreadArgc(argcId, isConstructing);
-  }
-
   writer.callClassHook(calleeObjId, argcId, hook, flags);
   writer.typeMonitorResult();
 
   if (templateObj) {
     writer.metaClassTemplateObject(templateObj, classOffset);
   }
 
   cacheIRStubKind_ = BaselineCacheIRStubKind::Monitored;
--- a/js/src/jit/CacheIR.h
+++ b/js/src/jit/CacheIR.h
@@ -243,17 +243,16 @@ extern const uint32_t ArgLengths[];
   _(GuardIndexIsValidUpdateOrAdd, Id, Id)                                      \
   _(GuardIndexGreaterThanDenseInitLength, Id, Id)                              \
   _(GuardTagNotEqual, Id, Id)                                                  \
   _(GuardXrayExpandoShapeAndDefaultProto, Id, Byte, Field)                     \
   _(GuardFunctionPrototype, Id, Id, Field)                                     \
   _(GuardNoAllocationMetadataBuilder, None)                                    \
   _(GuardObjectGroupNotPretenured, Field)                                      \
   _(GuardFunctionHasJitEntry, Id, Byte)                                        \
-  _(GuardAndUpdateSpreadArgc, Id, Byte)                                        \
   _(LoadObject, Id, Field)                                                     \
   _(LoadProto, Id, Id)                                                         \
   _(LoadEnclosingEnvironment, Id, Id)                                          \
   _(LoadWrapperTarget, Id, Id)                                                 \
   _(LoadValueTag, Id, Id)                                                      \
   _(LoadArgumentFixedSlot, Id, Byte)                                           \
   _(LoadArgumentDynamicSlot, Id, Id, Byte)                                     \
                                                                                \
@@ -838,24 +837,16 @@ class MOZ_RAII CacheIRWriter : public JS
     writeOp(CacheOp::GuardObjectGroupNotPretenured);
     addStubField(uintptr_t(group), StubField::Type::ObjectGroup);
   }
   void guardFunctionHasJitEntry(ObjOperandId fun, bool isConstructing) {
     writeOpWithOperandId(CacheOp::GuardFunctionHasJitEntry, fun);
     buffer_.writeByte(isConstructing);
   }
 
-  // NOTE: This must be the last guard before a call op, because it modifies
-  // argcReg in place (to avoid running out of registers on x86-32).
-  // TODO: fold this into the call op itself.
-  void guardAndUpdateSpreadArgc(Int32OperandId argcId, bool isConstructing) {
-    writeOpWithOperandId(CacheOp::GuardAndUpdateSpreadArgc, argcId);
-    buffer_.writeByte(isConstructing);
-  }
-
  public:
   // Use (or create) a specialization below to clarify what constaint the
   // group guard is implying.
   void guardGroup(ObjOperandId obj, ObjectGroup* group) {
     writeOpWithOperandId(CacheOp::GuardGroup, obj);
     addStubField(uintptr_t(group), StubField::Type::ObjectGroup);
   }
   void guardGroupForProto(ObjOperandId obj, ObjectGroup* group) {
--- a/js/src/jit/IonCacheIRCompiler.cpp
+++ b/js/src/jit/IonCacheIRCompiler.cpp
@@ -2608,19 +2608,15 @@ bool IonCacheIRCompiler::emitCallScripte
 bool IonCacheIRCompiler::emitCallNativeFunction() {
   MOZ_CRASH("Call ICs not used in ion");
 }
 
 bool IonCacheIRCompiler::emitCallClassHook() {
   MOZ_CRASH("Call ICs not used in ion");
 }
 
-bool IonCacheIRCompiler::emitGuardAndUpdateSpreadArgc() {
-  MOZ_CRASH("Call ICs not used in ion");
-}
-
 bool IonCacheIRCompiler::emitLoadArgumentFixedSlot() {
   MOZ_CRASH("Call ICs not used in ion");
 }
 
 bool IonCacheIRCompiler::emitLoadArgumentDynamicSlot() {
   MOZ_CRASH("Call ICs not used in ion");
 }