Last Comment Bug 707845 - IonMonkey: Better organization for VMFunctions
: IonMonkey: Better organization for VMFunctions
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: David Anderson [:dvander]
:
Mentors:
Depends on:
Blocks: 685097 708441
  Show dependency treegraph
 
Reported: 2011-12-05 17:50 PST by David Anderson [:dvander]
Modified: 2011-12-09 18:36 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1: simplify VMFunction (23.09 KB, patch)
2011-12-05 23:24 PST, David Anderson [:dvander]
nicolas.b.pierron: review+
Details | Diff | Splinter Review
part 2: simplify LIR helpers (7.98 KB, patch)
2011-12-05 23:33 PST, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
part 3: template magic (17.27 KB, patch)
2011-12-06 15:46 PST, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
part 3: different approach (16.66 KB, patch)
2011-12-06 16:31 PST, David Anderson [:dvander]
luke: review+
Details | Diff | Splinter Review
part 2: simplify LIR helpers (14.10 KB, patch)
2011-12-08 11:27 PST, David Anderson [:dvander]
nicolas.b.pierron: review-
Details | Diff | Splinter Review

Description David Anderson [:dvander] 2011-12-05 17:50:49 PST
Having the function signatures in jsinterp.h seems right but having the call information structures, as well as all of the VMFunction headers there too, is a little too invasive. At the very least we should separate these out into a separate header, but with Luke's help I'm going to try some template magic - either to automatically generate VMFunctions or to at least assert they're correct.
Comment 1 David Anderson [:dvander] 2011-12-05 19:30:01 PST
This should be possible if we remove FallibleType and slightly constrain what signatures can look like. If OutParam != None, the actual return type of the C++ function is bool. Otherwise, it's based on the return data type. This means that a signature that could return JSObject-or-null would have to be:

bool Something(JSContext *cx, JSObject **maybeObject);
Comment 2 David Anderson [:dvander] 2011-12-05 23:24:26 PST
Created attachment 579246 [details] [diff] [review]
part 1: simplify VMFunction

This patch simplifies the VMFunction structure and dependent code:
 * Existing VMFunction enumerations have been merged into one (DataType)
 * failType is now inferred from returnType+outParam
 * JSCCallMask -> CallMask
 * Register selection is simplified by just selecting (CallMask | JSCallMask)
 * VMFunctions are fallible by definition (otherwise they'd be ABI calls)
Comment 3 David Anderson [:dvander] 2011-12-05 23:33:22 PST
Created attachment 579247 [details] [diff] [review]
part 2: simplify LIR helpers

A few changes in this patch. The main goal was to separate LIR from VMFunctions.

 * Inheritors of LVMCallInstructionHelper no longer keep track of their
   VMFunction. This is because one LIR could select multiple VM functions
   (which was frequent in JM), or use the function only in an out-of-line
   path, bypassing defineVMReturn().
 * defineVMReturn() argument order uses the ordering from other define*,
   and extracts the return type from MIR instead of the VMFunction.

I forgot Luke's template code at the office, so I'll start on that tomorrow - even if it doesn't work out, I'd like to get these changes in before starting the series of GETPROP bugs.
Comment 4 Nicolas B. Pierron [:nbp] 2011-12-06 14:00:11 PST
(In reply to David Anderson [:dvander] from comment #2)
>  * VMFunctions are fallible by definition (otherwise they'd be ABI calls)

Don't we want unfallible VMFunctions where we have an exit frame to iterate the on stack ?
Comment 5 David Anderson [:dvander] 2011-12-06 14:28:19 PST
I think it's simpler to assume that anything in an exit frame is fallible. If we have infallible functions in an exit frame, they can just return true. I'm not sure how many functions would fall into this category, but I don't see any in JM.
Comment 6 Nicolas B. Pierron [:nbp] 2011-12-06 15:20:27 PST
Comment on attachment 579246 [details] [diff] [review]
part 1: simplify VMFunction

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

Replacing VMFunction::enum * by DataType is a good step forward, another option could be to rely on JS_TYPE in which case it would be easier to assert inside the defineVMReturn.

::: js/src/ion/IonLIR.h
@@ +831,2 @@
>            default:
>              JS_NOT_REACHED("Unknown ReturnType.");

Unexpected (Type_Void) instead of Unknown returnType.

::: js/src/ion/arm/Trampoline-arm.cpp
@@ -539,5 @@
>      }
>  
>      // Pick a register which is not among the return registers.
> -    regs = GeneralRegisterSet(Registers::AllocatableMask & ~Registers::JSCCallMask);
> -    temp = regs.takeAny();

Keep the temp assignment because It is used to shift the stack, or use scratch registers (like r12) instead of temp.

::: js/src/ion/x64/Trampoline-x64.cpp
@@ +468,5 @@
>  
>      masm.callWithABI(f.wrapped);
>  
>      // Test for failure.
> +    JS_ASSERT(f.failType() == VMFunction::Type_Bool || f.failType() == VMFunction::Type_Object);

This Assertion duplicates the default case of the next switch.

::: js/src/ion/x86/Architecture-x86.h
@@ +120,5 @@
>          (1 << JSC::X86Registers::ecx) |
>          (1 << JSC::X86Registers::edx);
>  
>      // Registers returned from a JS -> C call.
> +    static const uint32 CallMask =

ReturnMask & JSReturnMask ?  Otherwise I feel like we should include all volatile registers.

::: js/src/ion/x86/Trampoline-x86.cpp
@@ +487,4 @@
>  
>      // Load the outparam and free any allocated stack.
> +    if (f.outParam == VMFunction::Type_Value) {
> +        JS_ASSERT(f.returnType == VMFunction::Type_Bool);

This assertion would never be triggered because this check is included in the previous call of f.failType() done inside the previous JS_ASSERT.  This also apply to the other trampoline.

@@ +491,5 @@
>          masm.loadValue(Operand(esp, 0), JSReturnOperand);
>          masm.freeStack(sizeof(Value));
>      }
>  
> +    regs = GeneralRegisterSet::Not(GeneralRegisterSet(Registers::JSCallMask | Registers::CallMask));

You should keep the TODO about retn, because the only purpose of this mask is to get a temporary register to replace the return address before the return.

::: js/src/jsinterp.h
@@ +370,1 @@
>  struct VMFunction

nit: The comment style of the file is /* … */ not //.  Identically for following comments.

@@ +390,5 @@
> +
> +    // Identify which values are returned to the Jitted code. If and only if
> +    // the outParam is set to OutParam_Value, then the returnType must be
> +    // set to the ReturnValue.  Otherwise it should be either consistent
> +    // with the FallibleType or ReturnNothing.

Update the comment to follow DataType changes.

@@ +399,1 @@
>          return 1 + explicitArgs +

That was the reason why I reordered them in the first place ;)
Comment 7 Nicolas B. Pierron [:nbp] 2011-12-06 15:25:34 PST
Comment on attachment 579246 [details] [diff] [review]
part 1: simplify VMFunction

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

Oops, late fix reqest …

::: js/src/ion/x86/Trampoline-x86.cpp
@@ +452,5 @@
>      }
>  
>      // Reserve space for the outparameter.
>      Register outReg = InvalidReg;
> +    if (f.outParam == VMFunction::Type_Value) {

You should either implement other cases or assert that the outParam is Void in the else branch.  (This applies to the 3 Trampoline and for the freeStack in case you implement it)
Comment 8 David Anderson [:dvander] 2011-12-06 15:46:05 PST
Created attachment 579496 [details] [diff] [review]
part 3: template magic

Template magic based on sample code from Luke and suggestions from Nicolas. This lets us derive VMFunctions from function signatures. There's going to be some code duplication here because of template arity, but I think it will pay off. Now when a signature changes, callVM() sites will automatically break, and we don't have to worry about putting the VMFunction near the prototype.
Comment 9 Nicolas B. Pierron [:nbp] 2011-12-06 15:52:25 PST
Comment on attachment 579247 [details] [diff] [review]
part 2: simplify LIR helpers

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

Please update the assertions and consider using compiler branch prediction.

::: js/src/ion/IonLIR.h
@@ +818,1 @@
>              JS_ASSERT(Defs == 2);

This assertion is now wrong.  This should be BOX_PIECES instead of 2.

::: js/src/ion/Lowering.cpp
@@ +97,2 @@
>          return false;
>      if (!assignSafepoint(ins, lir))

I changed it in the first place to be similar to the assignSafepoint function, should we reorder the arguments of assignSafepoint too ?

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ -776,1 @@
>      masm.Push(type);

I added this assertion because explicitArgs are not defined in the same file and it seems safer to assert it now than having weird errors caused my mis-aligned stack.

Another way would be to use:

DebugOnly<uint32> stackState = masm.framePushed();
masm.Push(…);
masm.Push(…);
if (!callVM(NewInitArrayVMFun, ins))
    return false;
JS_ASSERT(stackState == masm.framePushed());

which cost one more line, but which can be safely copy & paste between callVM locations.

::: js/src/ion/shared/Lowering-shared-inl.h
@@ +154,5 @@
>      if (vreg >= MAX_VIRTUAL_REGISTERS)
>          return false;
>  
> +    switch (mir->type()) {
> +      case MIRType_Value:

Be aware that by removing the LVMCallInstructionHelper from the argument list and by replacing the switch condition would caused the switch statement to not be removed by the branch prediction of the compiler.

You can keep the LVMCallInstructionHelper template here to avoid this.

@@ -186,3 @@
>      }
>  
> -    JS_ASSERT(LDefinition::TypeFrom(mir->type()) == type);

By removing this assertion you remove a generic way to ensure that the VMFunction::returnType correspond to the MDefinition::type.  Add another way to check for this.
Comment 10 David Anderson [:dvander] 2011-12-06 16:31:35 PST
Created attachment 579519 [details] [diff] [review]
part 3: different approach

Nicolas tweaked this so that we don't have to re-declare the function signature.
Comment 11 David Anderson [:dvander] 2011-12-06 19:16:12 PST
> ::: js/src/ion/IonLIR.h
> @@ +818,1 @@
> >              JS_ASSERT(Defs == 2);
> 
> This assertion is now wrong.  This should be BOX_PIECES instead of 2.

Thanks, good catch.

> I changed it in the first place to be similar to the assignSafepoint
> function, should we reorder the arguments of assignSafepoint too ?

Sure, sounds good.

> I added this assertion because explicitArgs are not defined in the same file
> and it seems safer to assert it now than having weird errors caused my
> mis-aligned stack.

I think we can consider this covered by the third patch, which makes it a compile-time error if the signature of the function does not match the code below it. If you've, well, pushed the wrong number of arguments despite having the type-checked signature two lines above, then hopefully it gets caught by test cases they've written :)

> > -    JS_ASSERT(LDefinition::TypeFrom(mir->type()) == type);
> 
> By removing this assertion you remove a generic way to ensure that the
> VMFunction::returnType correspond to the MDefinition::type.  Add another way
> to check for this.

Okay, it looks like this can be added under callVM().
Comment 12 Luke Wagner [:luke] 2011-12-07 16:13:46 PST
Comment on attachment 579519 [details] [diff] [review]
part 3: different approach

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

Cool!

::: js/src/ion/IonVMFunctions.h
@@ +102,5 @@
> +    }
> +};
> +
> +template <class> struct TypeToDataType { };
> +template <> struct TypeToDataType<bool> { static const DataType value = Type_Bool; };

For consistency with js::tl, could you s/value/result/ ?

@@ +108,5 @@
> +template <class> struct OutParamToDataType { static const DataType value = Type_Void; };
> +template <> struct OutParamToDataType<Value *> { static const DataType value = Type_Value; };
> +
> +template <class R, class A1, class A2, R pf(JSContext *, A1, A2)>
> +struct FunctionInfo : public VMFunction {

You should be able to statically assert the outparam/returnValue invariant:

  typedef tl::StaticAssertIf<TypeToDataType<A2>::result == Type_Void,
                             TypeToDataType<R>::result != Type_Bool>::result _;

(adding tl::StaticAssertIf to js/TemplateLib.h net to tl::StaticAssert)

@@ +118,5 @@
> +    }
> +    static inline size_t explicitArgs() {
> +        return 2 + (outParam() != Type_Void ? 1 : 0);
> +    }
> +    FunctionInfo<R, A1, A2, pf>()

The name of a template refers to its current instantiation inside the template so you can just name the constructor with "FunctionInfo()".
Comment 13 David Anderson [:dvander] 2011-12-08 11:27:40 PST
Created attachment 580118 [details] [diff] [review]
part 2: simplify LIR helpers

Well I couldn't find a good way to add that assert back, but it doesn't seem worth bending over for. Return types are almost always inferred from MIR by define* variants, so really this would just assert that the function has the same effective return type as the MIR. (Also keep in mind one LIR could choose to call different VM functions during codegen.) And there would be bigger problems if you implemented, say, LUnboxDouble by doing callVM(NewArray).
Comment 14 Nicolas B. Pierron [:nbp] 2011-12-09 11:30:34 PST
Comment on attachment 580118 [details] [diff] [review]
part 2: simplify LIR helpers

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

This patch remove assertions and replace them by eye-proof checks.  I am not convinced by this patch.
I think you can still apply part 1 & part 2 patches without this one except for reordering the arguments of defineVMReturn and for replacing the assertion of the number of arguments by an assertion verifying the stack position before and after the call.

Removing these assertions will surely cause runtime error which would likely take time to investigate. (r=-)

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ -771,5 @@
>      // because it would be erased by the function call.
>      const Register type = ReturnReg;
>      masm.movePtr(ImmWord(ins->mir()->type()), type);
>  
> -    JS_ASSERT(ins->function().explicitArgs == 2);

This assertion is replaced by an eye check (in part 3 patch), which repeat arguments of the original function close by the Push-es made in the CodeGenerator.

::: js/src/ion/shared/Lowering-shared-inl.h
@@ -186,3 @@
>      }
>  
> -    JS_ASSERT(LDefinition::TypeFrom(mir->type()) == type);

This assertion is completely removed and it was used to ensure that the wrapper return type correspond to the Definition of the function.  Do you have something else to check for this ?
Comment 15 David Anderson [:dvander] 2011-12-09 18:36:13 PST
https://hg.mozilla.org/projects/ionmonkey/rev/75d1d99a8363
https://hg.mozilla.org/projects/ionmonkey/rev/b7096ab1699c
https://hg.mozilla.org/projects/ionmonkey/rev/5db40fd3d912
https://hg.mozilla.org/projects/ionmonkey/rev/d1aaf2fb79c7

talked with Nicolas, I added a new assert to check that the number of arguments pushed to callVM matches the explicit args of the function

Note You need to log in before you can comment on or make changes to this bug.