Closed Bug 707845 Opened 13 years ago Closed 12 years ago

IonMonkey: Better organization for VMFunctions

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dvander, Assigned: dvander)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
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);
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)
Attachment #579246 - Flags: review?(nicolas.b.pierron)
Attached patch part 2: simplify LIR helpers (obsolete) — Splinter Review
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.
Attachment #579247 - Flags: review?(nicolas.b.pierron)
(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 ?
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 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 ;)
Attachment #579246 - Flags: review?(nicolas.b.pierron) → review+
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)
Attached patch part 3: template magic (obsolete) — Splinter Review
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.
Attachment #579496 - Flags: review?(nicolas.b.pierron)
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.
Attachment #579247 - Flags: review?(nicolas.b.pierron)
Nicolas tweaked this so that we don't have to re-declare the function signature.
Attachment #579496 - Attachment is obsolete: true
Attachment #579496 - Flags: review?(nicolas.b.pierron)
Attachment #579519 - Flags: review?(luke)
> ::: 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().
Blocks: 685097
Blocks: 708441
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()".
Attachment #579519 - Flags: review?(luke) → review+
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).
Attachment #579247 - Attachment is obsolete: true
Attachment #580118 - Flags: review?(nicolas.b.pierron)
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 ?
Attachment #580118 - Flags: review?(nicolas.b.pierron) → review-
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.