878374 - IonMonkey: Ability to have non-Value MutableHandles as outparam of VMFunctions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Shu-yu Guo [:shu]]

Currently, only MutableHandleValue is allowed as outparam for VMFunctions. For parallel execution, we always return a ParallelResult, and the outparam may be other MutableHandle types as well.

Comment 1

[Shu-yu Guo [:shu]]

Attachment: v0 for x64 only

This only updates the x64-related files. If the approach checks out, I'll implement the other 2 platforms.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 756903 [details] [diff] [review]
v0 for x64 only

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

::: js/src/ion/shared/IonFrames-x86-shared.h
@@ +153,1 @@
>      Value *outVp() {

It sounds like both can be refactored into:

  template <typename T>
  T *outParam() {
      return reinterpret_cast<T *>(reinterpret_cast<char *>(this) - sizeof(T));
  }

in which case the code inside IonFrames.cpp can become

  gc::MarkObjectRoot(trc, footer->outParam<JSFunction *>(), "ion-vm-out");
   …
  gc::MarkValueRoot(trc, footer->outParam<Value>(), "ion-vm-out");

::: js/src/ion/x64/Trampoline-x64.cpp
@@ +536,5 @@
> +          case VMFunction::RootString:
> +          case VMFunction::RootPropertyName:
> +          case VMFunction::RootFunction:
> +          case VMFunction::RootCell:
> +            masm.reserveStack(sizeof(void *));

All of these are pointers which are read during the marking phase.  Even if the current use will only be in parallel execution mode with no GC, I would prefer to see these pointers initialized to NULL instead of reusing the current unknown stack value.

  masm.Push(NULL);

Also, it sounds like these pieces of code would be identical on all architectures, might be worth putting them in IonMacroAssembler.cpp by giving a "const VMFunction &" as argument.

Comment 3

[Shu-yu Guo [:shu]]

Thanks for feedback, I'll incorporate your suggestions.

Comment 4

[Shu-yu Guo [:shu]]

Attachment: v1

Applied nbp's refactors. Carrying r+.

Comment 5

[Shu-yu Guo [:shu]]

Comment on attachment 757755 [details] [diff] [review]
v1

>From: Shu-yu Guo <[e-mail]>
>Date:   Fri May 31 19:16:14 2013 -0700
>
>Bug 878374 - Support non-Value Handles as VMFunction outparams. (r=nbp)
>
>diff --git a/js/src/ion/IonFrames.cpp b/js/src/ion/IonFrames.cpp
>--- a/js/src/ion/IonFrames.cpp
>+++ b/js/src/ion/IonFrames.cpp
>@@ -793,20 +793,36 @@ IonActivationIterator::ionStackRange(uin
>     IonFrameIterator frames(top());
> 
>     if (frames.isFakeExitFrame()) {
>         min = reinterpret_cast<uintptr_t *>(frames.fp());
>     } else {
>         IonExitFrameLayout *exitFrame = frames.exitFrame();
>         IonExitFooterFrame *footer = exitFrame->footer();
>         const VMFunction *f = footer->function();
>-        if (exitFrame->isWrapperExit() && f->outParam == Type_Handle)
>-            min = reinterpret_cast<uintptr_t *>(footer->outVp());
>-        else
>+        if (exitFrame->isWrapperExit() && f->outParam == Type_Handle) {
>+            switch (f->outParamRootType) {
>+              case VMFunction::RootNone:
>+                JS_NOT_REACHED("Handle outparam must have root type");
>+                break;
>+              case VMFunction::RootObject:
>+              case VMFunction::RootString:
>+              case VMFunction::RootPropertyName:
>+              case VMFunction::RootFunction:
>+              case VMFunction::RootCell:
>+                // These are all handles to GCThing pointers.
>+                min = reinterpret_cast<uintptr_t *>(footer->outParam<void *>());
>+                break;
>+              case VMFunction::RootValue:
>+                min = reinterpret_cast<uintptr_t *>(footer->outParam<Value>());
>+                break;
>+            }
>+        } else {
>             min = reinterpret_cast<uintptr_t *>(footer);
>+        }
>     }
> 
>     while (!frames.done())
>         ++frames;
> 
>     end = reinterpret_cast<uintptr_t *>(frames.prevFp());
> }
> 
>@@ -920,18 +936,39 @@ MarkIonExitFrame(JSTracer *trc, const Io
>             break;
>           case VMFunction::DoubleByValue:
>           case VMFunction::DoubleByRef:
>             argBase += 2 * sizeof(void *);
>             break;
>         }
>     }
> 
>-    if (f->outParam == Type_Handle)
>-        gc::MarkValueRoot(trc, footer->outVp(), "ion-vm-outvp");
>+    if (f->outParam == Type_Handle) {
>+        switch (f->outParamRootType) {
>+          case VMFunction::RootNone:
>+            JS_NOT_REACHED("Handle outparam must have root type");
>+            break;
>+          case VMFunction::RootObject:
>+            gc::MarkObjectRoot(trc, footer->outParam<JSObject *>(), "ion-vm-out");
>+            break;
>+          case VMFunction::RootString:
>+          case VMFunction::RootPropertyName:
>+            gc::MarkStringRoot(trc, footer->outParam<JSString *>(), "ion-vm-out");
>+            break;
>+          case VMFunction::RootFunction:
>+            gc::MarkObjectRoot(trc, footer->outParam<JSFunction *>(), "ion-vm-out");
>+            break;
>+          case VMFunction::RootValue:
>+            gc::MarkValueRoot(trc, footer->outParam<Value>(), "ion-vm-outvp");
>+            break;
>+          case VMFunction::RootCell:
>+            gc::MarkGCThingRoot(trc, footer->outParam<void *>(), "ion-vm-out");
>+            break;
>+        }
>+    }
> }
> 
> static void
> MarkIonActivation(JSTracer *trc, const IonActivationIterator &activations)
> {
>     for (IonFrameIterator frames(activations); !frames.done(); ++frames) {
>         switch (frames.type()) {
>           case IonFrame_Exit:
>diff --git a/js/src/ion/IonMacroAssembler.cpp b/js/src/ion/IonMacroAssembler.cpp
>--- a/js/src/ion/IonMacroAssembler.cpp
>+++ b/js/src/ion/IonMacroAssembler.cpp
>@@ -1087,16 +1087,63 @@ void
> MacroAssembler::convertInt32ValueToDouble(const Address &address, Register scratch, Label *done)
> {
>     branchTestInt32(Assembler::NotEqual, address, done);
>     unboxInt32(address, scratch);
>     convertInt32ToDouble(scratch, ScratchFloatReg);
>     storeDouble(ScratchFloatReg, address);
> }
> 
>+void
>+MacroAssembler::PushEmptyHandle(VMFunction::RootType rootType)
>+{
>+    switch (rootType) {
>+      case VMFunction::RootNone:
>+        JS_NOT_REACHED("Handle must have root type");
>+        break;
>+      case VMFunction::RootObject:
>+      case VMFunction::RootString:
>+      case VMFunction::RootPropertyName:
>+      case VMFunction::RootFunction:
>+      case VMFunction::RootCell:
>+        Push(ImmWord((void *)NULL));
>+        break;
>+      case VMFunction::RootValue:
>+        Push(UndefinedValue());
>+        break;
>+    }
>+}
>+
>+TypedOrValueRegister
>+MacroAssembler::loadHandle(VMFunction::RootType rootType, Address src,
>+                           Register cellReg, const ValueOperand &valueReg)
>+{
>+    switch (rootType) {
>+      case VMFunction::RootNone:
>+        break;
>+      case VMFunction::RootObject:
>+      case VMFunction::RootFunction:
>+        loadPtr(src, cellReg);
>+        return TypedOrValueRegister(MIRType_Object, AnyRegister(cellReg));
>+      case VMFunction::RootString:
>+      case VMFunction::RootPropertyName:
>+        loadPtr(src, cellReg);
>+        return TypedOrValueRegister(MIRType_String, AnyRegister(cellReg));
>+      case VMFunction::RootCell:
>+        loadPtr(src, cellReg);
>+        return TypedOrValueRegister(MIRType_None, AnyRegister(cellReg));
>+      case VMFunction::RootValue:
>+        loadValue(src, valueReg);
>+        return TypedOrValueRegister(valueReg);
>+    }
>+    // Placed here to silence warning.
>+    JS_NOT_REACHED("Handle must have root type");
>+    return TypedOrValueRegister();
>+}
>+
> #ifdef JS_ASMJS
> ABIArgIter::ABIArgIter(const MIRTypeVector &types)
>   : gen_(),
>     types_(types),
>     i_(0)
> {
>     if (!done())
>         gen_.next(types_[i_]);
>diff --git a/js/src/ion/IonMacroAssembler.h b/js/src/ion/IonMacroAssembler.h
>--- a/js/src/ion/IonMacroAssembler.h
>+++ b/js/src/ion/IonMacroAssembler.h
>@@ -12,16 +12,17 @@
> #elif defined(JS_CPU_X64)
> # include "ion/x64/MacroAssembler-x64.h"
> #elif defined(JS_CPU_ARM)
> # include "ion/arm/MacroAssembler-arm.h"
> #endif
> #include "ion/IonCompartment.h"
> #include "ion/IonInstrumentation.h"
> #include "ion/ParallelFunctions.h"
>+#include "ion/VMFunctions.h"
> 
> #include "vm/ForkJoin.h"
> 
> #include "jstypedarray.h"
> #include "jscompartment.h"
> 
> #include "vm/Shape.h"
> 
>@@ -409,16 +410,20 @@ class MacroAssembler : public MacroAssem
>     }
> 
>     void PushValue(const Address &addr) {
>         JS_ASSERT(addr.base != StackPointer);
>         pushValue(addr);
>         framePushed_ += sizeof(Value);
>     }
> 
>+    void PushEmptyHandle(VMFunction::RootType rootType);
>+    TypedOrValueRegister loadHandle(VMFunction::RootType rootType, Address src,
>+                                    Register cellReg, const ValueOperand &valueReg);
>+
>     void adjustStack(int amount) {
>         if (amount > 0)
>             freeStack(amount);
>         else if (amount < 0)
>             reserveStack(-amount);
>     }
> 
>     void bumpKey(Int32Key *key, int diff) {
>diff --git a/js/src/ion/VMFunctions.cpp b/js/src/ion/VMFunctions.cpp
>--- a/js/src/ion/VMFunctions.cpp
>+++ b/js/src/ion/VMFunctions.cpp
>@@ -42,16 +42,36 @@ VMFunction::addToFunctions()
>     if (!initialized) {
>         initialized = true;
>         functions = NULL;
>     }
>     this->next = functions;
>     functions = this;
> }
> 
>+/* static */ size_t
>+VMFunction::sizeOfRootType(RootType type)
>+{
>+    switch (type) {
>+      case RootNone:
>+        break;
>+      case RootObject:
>+      case RootString:
>+      case RootPropertyName:
>+      case RootFunction:
>+      case RootCell:
>+        return sizeof(void *);
>+      case RootValue:
>+        return sizeof(Value);
>+    }
>+    // Placed here to silence warning.
>+    JS_NOT_REACHED("Handle must have root type");
>+    return 0;
>+}
>+
> bool
> InvokeFunction(JSContext *cx, HandleFunction fun0, uint32_t argc, Value *argv, Value *rval)
> {
>     RootedFunction fun(cx, fun0);
>     if (fun->isInterpreted()) {
>         if (fun->isInterpretedLazy() && !fun->getOrCreateScript(cx))
>             return false;
> 
>diff --git a/js/src/ion/VMFunctions.h b/js/src/ion/VMFunctions.h
>--- a/js/src/ion/VMFunctions.h
>+++ b/js/src/ion/VMFunctions.h
>@@ -99,16 +99,19 @@ struct VMFunction
>         RootValue,
>         RootCell
>     };
> 
>     // Contains an combination of enumerated types used by the gc for marking
>     // arguments of the VM wrapper.
>     uint64_t argumentRootTypes;
> 
>+    // The root type of the out param if outParam == Type_Handle.
>+    RootType outParamRootType;
>+
>     // Does this function take a ForkJoinSlice * or a JSContext *?
>     ExecutionMode executionMode;
> 
>     // Number of Values the VM wrapper should pop from the stack when it returns.
>     // Used by baseline IC stubs so that they can use tail calls to call the VM
>     // wrapper.
>     uint32_t extraValuesToPop;
> 
>@@ -180,33 +183,35 @@ struct VMFunction
> 
>     VMFunction()
>       : wrapped(NULL),
>         explicitArgs(0),
>         argumentProperties(0),
>         argumentPassedInFloatRegs(0),
>         outParam(Type_Void),
>         returnType(Type_Void),
>+        outParamRootType(RootNone),
>         executionMode(SequentialExecution),
>         extraValuesToPop(0)
>     {
>     }
> 
> 
>     VMFunction(void *wrapped, uint32_t explicitArgs, uint32_t argumentProperties,
>                uint32_t argumentPassedInFloatRegs, uint64_t argRootTypes,
>-               DataType outParam, DataType returnType,
>+               DataType outParam, RootType outParamRootType, DataType returnType,
>                ExecutionMode executionMode, uint32_t extraValuesToPop = 0)
>       : wrapped(wrapped),
>         explicitArgs(explicitArgs),
>         argumentProperties(argumentProperties),
>         argumentPassedInFloatRegs(argumentPassedInFloatRegs),
>         outParam(outParam),
>         returnType(returnType),
>         argumentRootTypes(argRootTypes),
>+        outParamRootType(outParamRootType),
>         executionMode(executionMode),
>         extraValuesToPop(extraValuesToPop)
>     {
>         // Check for valid failure/return type.
>         JS_ASSERT_IF(outParam != Type_Void && executionMode == SequentialExecution,
>                      returnType == Type_Bool);
>         JS_ASSERT_IF(executionMode == ParallelExecution, returnType == Type_ParallelResult);
>         JS_ASSERT(returnType == Type_Bool ||
>@@ -215,16 +220,18 @@ struct VMFunction
>     }
> 
>     VMFunction(const VMFunction &o)
>     {
>         *this = o;
>         addToFunctions();
>     }
> 
>+    static size_t sizeOfRootType(RootType type);
>+
>   private:
>     // Add this to the global list of VMFunctions.
>     void addToFunctions();
> };
> 
> template <class> struct TypeToDataType { /* Unexpected return type for a VMFunction. */ };
> template <> struct TypeToDataType<bool> { static const DataType result = Type_Bool; };
> template <> struct TypeToDataType<JSObject *> { static const DataType result = Type_Object; };
>@@ -320,16 +327,29 @@ template <> struct TypeToRootType<Handle
> 
> template <class> struct OutParamToDataType { static const DataType result = Type_Void; };
> template <> struct OutParamToDataType<Value *> { static const DataType result = Type_Value; };
> template <> struct OutParamToDataType<int *> { static const DataType result = Type_Int32; };
> template <> struct OutParamToDataType<uint32_t *> { static const DataType result = Type_Int32; };
> template <> struct OutParamToDataType<MutableHandleValue> { static const DataType result = Type_Handle; };
> template <> struct OutParamToDataType<MutableHandleObject> { static const DataType result = Type_Handle; };
> 
>+template <class> struct OutParamToRootType {
>+    static const VMFunction::RootType result = VMFunction::RootNone;
>+};
>+template <> struct OutParamToRootType<MutableHandleValue> {
>+    static const VMFunction::RootType result = VMFunction::RootValue;
>+};
>+template <> struct OutParamToRootType<MutableHandleObject> {
>+    static const VMFunction::RootType result = VMFunction::RootObject;
>+};
>+template <> struct OutParamToRootType<MutableHandleString> {
>+    static const VMFunction::RootType result = VMFunction::RootString;
>+};
>+
> template <class> struct MatchContext { };
> template <> struct MatchContext<JSContext *> {
>     static const ExecutionMode execMode = SequentialExecution;
> };
> template <> struct MatchContext<ForkJoinSlice *> {
>     static const ExecutionMode execMode = ParallelExecution;
> };
> 
>@@ -337,32 +357,36 @@ template <> struct MatchContext<ForkJoin
> #define FOR_EACH_ARGS_2(Macro, Sep, Last) FOR_EACH_ARGS_1(Macro, Sep, Sep) Macro(2) Last(2)
> #define FOR_EACH_ARGS_3(Macro, Sep, Last) FOR_EACH_ARGS_2(Macro, Sep, Sep) Macro(3) Last(3)
> #define FOR_EACH_ARGS_4(Macro, Sep, Last) FOR_EACH_ARGS_3(Macro, Sep, Sep) Macro(4) Last(4)
> #define FOR_EACH_ARGS_5(Macro, Sep, Last) FOR_EACH_ARGS_4(Macro, Sep, Sep) Macro(5) Last(5)
> #define FOR_EACH_ARGS_6(Macro, Sep, Last) FOR_EACH_ARGS_5(Macro, Sep, Sep) Macro(6) Last(6)
> 
> #define COMPUTE_INDEX(NbArg) NbArg
> #define COMPUTE_OUTPARAM_RESULT(NbArg) OutParamToDataType<A ## NbArg>::result
>+#define COMPUTE_OUTPARAM_ROOT(NbArg) OutParamToRootType<A ## NbArg>::result
> #define COMPUTE_ARG_PROP(NbArg) (TypeToArgProperties<A ## NbArg>::result << (2 * (NbArg - 1)))
> #define COMPUTE_ARG_ROOT(NbArg) (uint64_t(TypeToRootType<A ## NbArg>::result) << (3 * (NbArg - 1)))
> #define COMPUTE_ARG_FLOAT(NbArg) (TypeToPassInFloatReg<A ## NbArg>::result) << (NbArg - 1)
> #define SEP_OR(_) |
> #define NOTHING(_)
> 
> #define FUNCTION_INFO_STRUCT_BODY(ForEachNb)                                            \
>     static inline ExecutionMode executionMode() {                                       \
>         return MatchContext<Context>::execMode;                                         \
>     }                                                                                   \
>     static inline DataType returnType() {                                               \
>         return TypeToDataType<R>::result;                                               \
>     }                                                                                   \
>     static inline DataType outParam() {                                                 \
>         return ForEachNb(NOTHING, NOTHING, COMPUTE_OUTPARAM_RESULT);                    \
>     }                                                                                   \
>+    static inline RootType outParamRootType() {                                         \
>+        return ForEachNb(NOTHING, NOTHING, COMPUTE_OUTPARAM_ROOT);                      \
>+    }                                                                                   \
>     static inline size_t NbArgs() {                                                     \
>         return ForEachNb(NOTHING, NOTHING, COMPUTE_INDEX);                              \
>     }                                                                                   \
>     static inline size_t explicitArgs() {                                               \
>         return NbArgs() - (outParam() != Type_Void ? 1 : 0);                            \
>     }                                                                                   \
>     static inline uint32_t argumentProperties() {                                       \
>         return ForEachNb(COMPUTE_ARG_PROP, SEP_OR, NOTHING);                            \
>@@ -371,18 +395,18 @@ template <> struct MatchContext<ForkJoin
>         return ForEachNb(COMPUTE_ARG_FLOAT, SEP_OR, NOTHING);                           \
>     }                                                                                   \
>     static inline uint64_t argumentRootTypes() {                                        \
>         return ForEachNb(COMPUTE_ARG_ROOT, SEP_OR, NOTHING);                            \
>     }                                                                                   \
>     FunctionInfo(pf fun, PopValues extraValuesToPop = PopValues(0))                     \
>         : VMFunction(JS_FUNC_TO_DATA_PTR(void *, fun), explicitArgs(),                  \
>                      argumentProperties(), argumentPassedInFloatRegs(),                 \
>-                     argumentRootTypes(),                                               \
>-                     outParam(), returnType(), executionMode(),                         \
>+                     argumentRootTypes(), outParam(), outParamRootType(),               \
>+                     returnType(), executionMode(),                                     \
>                      extraValuesToPop.numValues)                                        \
>     { }
> 
> template <typename Fun>
> struct FunctionInfo {
> };
> 
> // VMFunction wrapper with no explicit arguments.
>@@ -394,33 +418,36 @@ struct FunctionInfo<R (*)(Context)> : pu
>         return MatchContext<Context>::execMode;
>     }
>     static inline DataType returnType() {
>         return TypeToDataType<R>::result;
>     }
>     static inline DataType outParam() {
>         return Type_Void;
>     }
>+    static inline RootType outParamRootType() {
>+        return RootNone;
>+    }
>     static inline size_t explicitArgs() {
>         return 0;
>     }
>     static inline uint32_t argumentProperties() {
>         return 0;
>     }
>     static inline uint32_t argumentPassedInFloatRegs() {
>         return 0;
>     }
>     static inline uint64_t argumentRootTypes() {
>         return 0;
>     }
>     FunctionInfo(pf fun)
>       : VMFunction(JS_FUNC_TO_DATA_PTR(void *, fun), explicitArgs(),
>                    argumentProperties(), argumentPassedInFloatRegs(),
>-                   argumentRootTypes(),
>-                   outParam(), returnType(), executionMode())
>+                   argumentRootTypes(), outParam(), outParamRootType(),
>+                   returnType(), executionMode())
>     { }
> };
> 
> // Specialize the class for each number of argument used by VMFunction.
> // Keep it verbose unless you find a readable macro for it.
> template <class R, class Context, class A1>
> struct FunctionInfo<R (*)(Context, A1)> : public VMFunction {
>     typedef R (*pf)(Context, A1);
>@@ -463,16 +490,17 @@ template <class R, class Context, class 
> #undef FOR_EACH_ARGS_5
> #undef FOR_EACH_ARGS_4
> #undef FOR_EACH_ARGS_3
> #undef FOR_EACH_ARGS_2
> #undef FOR_EACH_ARGS_1
> 
> #undef COMPUTE_INDEX
> #undef COMPUTE_OUTPARAM_RESULT
>+#undef COMPUTE_OUTPARAM_ROOT
> #undef COMPUTE_ARG_PROP
> #undef COMPUTE_ARG_FLOAT
> #undef SEP_OR
> #undef NOTHING
> 
> class AutoDetectInvalidation
> {
>     JSContext *cx_;
>diff --git a/js/src/ion/arm/IonFrames-arm.h b/js/src/ion/arm/IonFrames-arm.h
>--- a/js/src/ion/arm/IonFrames-arm.h
>+++ b/js/src/ion/arm/IonFrames-arm.h
>@@ -149,18 +149,19 @@ class IonExitFooterFrame
>     inline IonCode **addressOfIonCode() {
>         return &ionCode_;
>     }
>     inline const VMFunction *function() const {
>         return function_;
>     }
> 
>     // This should only be called for function()->outParam == Type_Handle
>-    Value *outVp() {
>-        return reinterpret_cast<Value *>(reinterpret_cast<char *>(this) - sizeof(Value));
>+    template <typename T>
>+    T *outParam() {
>+        return reinterpret_cast<T *>(reinterpret_cast<char *>(this) - sizeof(T));
>     }
> };
> 
> class IonOsrFrameLayout : public IonJSFrameLayout
> {
>   public:
>     static inline size_t Size() {
>         return sizeof(IonOsrFrameLayout);
>diff --git a/js/src/ion/arm/Trampoline-arm.cpp b/js/src/ion/arm/Trampoline-arm.cpp
>--- a/js/src/ion/arm/Trampoline-arm.cpp
>+++ b/js/src/ion/arm/Trampoline-arm.cpp
>@@ -624,17 +624,17 @@ IonRuntime::generateVMWrapper(JSContext 
>         regs.take(outReg);
>         masm.reserveStack(sizeof(Value));
>         masm.ma_mov(sp, outReg);
>         break;
> 
>       case Type_Handle:
>         outReg = r4;
>         regs.take(outReg);
>-        masm.Push(UndefinedValue());
>+        masm.PushEmptyHandle(f.outParamRootType);
>         masm.ma_mov(sp, outReg);
>         break;
> 
>       case Type_Int32:
>         outReg = r4;
>         regs.take(outReg);
>         masm.reserveStack(sizeof(int32_t));
>         masm.ma_mov(sp, outReg);
>@@ -694,16 +694,20 @@ IonRuntime::generateVMWrapper(JSContext 
>       default:
>         JS_NOT_REACHED("unknown failure kind");
>         break;
>     }
> 
>     // Load the outparam and free any allocated stack.
>     switch (f.outParam) {
>       case Type_Handle:
>+        masm.loadHandle(f.outParamRootType, Address(sp, 0), ReturnReg, JSReturnOperand);
>+        masm.freeStack(VMFunction::sizeOfRootType(f.outParamRootType));
>+        break;
>+
>       case Type_Value:
>         masm.loadValue(Address(sp, 0), JSReturnOperand);
>         masm.freeStack(sizeof(Value));
>         break;
> 
>       case Type_Int32:
>         masm.load32(Address(sp, 0), ReturnReg);
>         masm.freeStack(sizeof(int32_t));
>diff --git a/js/src/ion/shared/IonFrames-x86-shared.h b/js/src/ion/shared/IonFrames-x86-shared.h
>--- a/js/src/ion/shared/IonFrames-x86-shared.h
>+++ b/js/src/ion/shared/IonFrames-x86-shared.h
>@@ -142,18 +142,19 @@ class IonExitFooterFrame
>     inline IonCode **addressOfIonCode() {
>         return &ionCode_;
>     }
>     inline const VMFunction *function() const {
>         return function_;
>     }
> 
>     // This should only be called for function()->outParam == Type_Handle
>-    Value *outVp() {
>-        return reinterpret_cast<Value *>(reinterpret_cast<char *>(this) - sizeof(Value));
>+    template <typename T>
>+    T *outParam() {
>+        return reinterpret_cast<T *>(reinterpret_cast<char *>(this) - sizeof(T));
>     }
> };
> 
> class IonNativeExitFrameLayout;
> class IonOOLNativeGetterExitFrameLayout;
> class IonOOLPropertyOpExitFrameLayout;
> class IonOOLProxyGetExitFrameLayout;
> class IonDOMExitFrameLayout;
>diff --git a/js/src/ion/x64/Trampoline-x64.cpp b/js/src/ion/x64/Trampoline-x64.cpp
>--- a/js/src/ion/x64/Trampoline-x64.cpp
>+++ b/js/src/ion/x64/Trampoline-x64.cpp
>@@ -523,17 +523,17 @@ IonRuntime::generateVMWrapper(JSContext 
>       case Type_Value:
>         outReg = regs.takeAny();
>         masm.reserveStack(sizeof(Value));
>         masm.movq(esp, outReg);
>         break;
> 
>       case Type_Handle:
>         outReg = regs.takeAny();
>-        masm.Push(UndefinedValue());
>+        masm.PushEmptyHandle(f.outParamRootType);
>         masm.movq(esp, outReg);
>         break;
> 
>       case Type_Int32:
>         outReg = regs.takeAny();
>         masm.reserveStack(sizeof(int32_t));
>         masm.movq(esp, outReg);
>         break;
>@@ -594,16 +594,20 @@ IonRuntime::generateVMWrapper(JSContext 
>       default:
>         JS_NOT_REACHED("unknown failure kind");
>         break;
>     }
> 
>     // Load the outparam and free any allocated stack.
>     switch (f.outParam) {
>       case Type_Handle:
>+        masm.loadHandle(f.outParamRootType, Address(esp, 0), ReturnReg, JSReturnOperand);
>+        masm.freeStack(VMFunction::sizeOfRootType(f.outParamRootType));
>+        break;
>+
>       case Type_Value:
>         masm.loadValue(Address(esp, 0), JSReturnOperand);
>         masm.freeStack(sizeof(Value));
>         break;
> 
>       case Type_Int32:
>         masm.load32(Address(esp, 0), ReturnReg);
>         masm.freeStack(sizeof(int32_t));
>diff --git a/js/src/ion/x86/Trampoline-x86.cpp b/js/src/ion/x86/Trampoline-x86.cpp
>--- a/js/src/ion/x86/Trampoline-x86.cpp
>+++ b/js/src/ion/x86/Trampoline-x86.cpp
>@@ -540,17 +540,17 @@ IonRuntime::generateVMWrapper(JSContext 
>       case Type_Value:
>         outReg = regs.takeAny();
>         masm.reserveStack(sizeof(Value));
>         masm.movl(esp, outReg);
>         break;
> 
>       case Type_Handle:
>         outReg = regs.takeAny();
>-        masm.Push(UndefinedValue());
>+        masm.PushEmptyHandle(f.outParamRootType);
>         masm.movl(esp, outReg);
>         break;
> 
>       case Type_Int32:
>         outReg = regs.takeAny();
>         masm.reserveStack(sizeof(int32_t));
>         masm.movl(esp, outReg);
>         break;
>@@ -616,16 +616,20 @@ IonRuntime::generateVMWrapper(JSContext 
>       default:
>         JS_NOT_REACHED("unknown failure kind");
>         break;
>     }
> 
>     // Load the outparam and free any allocated stack.
>     switch (f.outParam) {
>       case Type_Handle:
>+        masm.loadHandle(f.outParamRootType, Address(esp, 0), ReturnReg, JSReturnOperand);
>+        masm.freeStack(VMFunction::sizeOfRootType(f.outParamRootType));
>+        break;
>+
>       case Type_Value:
>         masm.loadValue(Address(esp, 0), JSReturnOperand);
>         masm.freeStack(sizeof(Value));
>         break;
> 
>       case Type_Int32:
>         masm.load32(Address(esp, 0), ReturnReg);
>         masm.freeStack(sizeof(JSBool));

Comment 6

[Shu-yu Guo [:shu]]

Holy crap why is the entire patch in the comments

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 757755 [details] [diff] [review]
v1

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

Apply the following nits and r=me.

::: js/src/ion/IonMacroAssembler.cpp
@@ +1092,5 @@
>      storeDouble(ScratchFloatReg, address);
>  }
>  
> +void
> +MacroAssembler::PushEmptyHandle(VMFunction::RootType rootType)

nit: PushEmptyRooted
  the handle-like value is the stack pointer that we copy after.

@@ +1113,5 @@
> +}
> +
> +TypedOrValueRegister
> +MacroAssembler::loadHandle(VMFunction::RootType rootType, Address src,
> +                           Register cellReg, const ValueOperand &valueReg)

I think you can omit the returned value.  In addition, we can omit the addition of sizeOfRootType and freeStack by renaming this function popRooted.

	masm.loadHandle(f.outParamRootType, Address(sp, 0), ReturnReg, JSReturnOperand);
	masm.freeStack(VMFunction::sizeOfRootType(f.outParamRootType));
	break;

Comment 8

[Shu-yu Guo [:shu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/459962a42baa

try: https://tbpl.mozilla.org/?tree=Try&rev=4809ded80471

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/459962a42baa