Last Comment Bug 689325 - IonMonkey: Insert type guards on TypeSets attached to MParameters
: IonMonkey: Insert type guards on TypeSets attached to MParameters
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: 688174 689831
Blocks: 684402
  Show dependency treegraph
 
Reported: 2011-09-26 15:04 PDT by David Anderson [:dvander]
Modified: 2011-10-24 18:51 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip v0 (29.62 KB, patch)
2011-09-27 00:02 PDT, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
wip v1 (51.45 KB, patch)
2011-10-06 18:52 PDT, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
part 1: everything but ARM (83.49 KB, patch)
2011-10-07 17:45 PDT, David Anderson [:dvander]
sstangl: review+
bhackett1024: review+
Details | Diff | Splinter Review

Description David Anderson [:dvander] 2011-09-26 15:04:41 PDT
We're not correct wrt TI until we can do this. We'd like for functions to have two entry points: one that simply unboxes instructions (knowing that its caller provided the correct types), and another that actually performs checks.

One idea: in the method, treat TI info as guaranteed. Unbox parameters, don't put guards. However, create a separate IonCode per-method trampoline that *will* perform the guards, creating a new (temporary) "argument type check" frame. If the type checks pass, it'll remove its frame and call into the desired function. If any check fails, we'd go back to the interpreter.

A few downsides to this approach:
 (a) It's potentially aggressive, though we could change it to an IC later.
 (b) It introduces a new Ion frame type that has special meaning on failure.

On the upside, it fits nicely into our model, and would work right now. A few other options:

(1) Attach a second entry-point with hand-generated checks. This would complicate
    generic function calls since they'd have to add (code->raw + check_offset),
    alternately we could add a relocation type that understands interior pointers.
(2) Attach a second entry point for not having checks. This would have similar
    problems, but they might require less work to tackle.
(3) Two entry-points at the MIR+LIR level but this strikes me as messy.
Comment 1 David Anderson [:dvander] 2011-09-26 15:07:49 PDT
(4) Always do the argument checking and put off the optimal thing for a separate bug, but that feels wrong since we'll end up just tearing that code out.
Comment 2 Sean Stangl [:sstangl] 2011-09-26 17:31:07 PDT
Disregarding recompilation, there are three problems to address:
 1. How to attach type barriers onto the MIR;
 2. How to integrate type information on parameters into the MIR;
 3. How to provide a second entry point that skips type barriers on parameters.

After some iteration, I now believe the following:

1. Using MUnbox is reasonable to express a typed MParameter, but has a couple of unwanted implications: first, unboxing inserts a guard with bailout, but the whole point of the initial barrier is that we know this step succeeds unconditionally; second, MUnbox has regalloc implications -- we really want the data to be loaded as close to its uses as possible.

We can skirt these issues by just expressing TI information via the MIRType of the MParameter, which makes everything just work. We never actually need explicit unboxing to make use of TI.

2. Barriers should execute at point-of-definition. For example, if we have a JSOP_CALLARG in a loop, and TI tells us that the callsite is monomorphic with a particular JSFunction, the (not just type!) barrier should exist on the MParameter, not at the callsite.

This requirement implies that barriers are created by the users of an MDefinition, not at the point at which an MDefinition exists. Then a barrier exists iff the generated code depends on that invariant, and all barriers are at point-of-definition. So we need a way for barriers to be tracked against an MDefinition, and a way to update those barriers. We could hijack such a mechanism to automatically handle TypeSet freezing, to ensure that we don't over-freeze, and to eliminate possible bugs where we forget to freeze.

3. The initial trampoline that executes MParameter barrier checks should then be generated by iterating over the MParameters as the first step of codegen, which requires no modification, and doesn't *really* have to be a separate trampoline, unless it's easier to think of it that way. We can then insert a label, or some equivalent, to mark the absolute address at which these checks have finished; the address would be stored in the IonScript.

Note that this should not require a separate frame to handle argument checking: we can just construct the function's normal frame, and make barrier failure a regular bailout to the MResumePoint at the head of the function, possibly with some invalidation magic.
Comment 3 David Anderson [:dvander] 2011-09-26 18:30:32 PDT
> 1. Using MUnbox is reasonable to express a typed MParameter, but has a
> couple of unwanted implications: first, unboxing inserts a guard with
> bailout, but the whole point of the initial barrier is that we know this
> step succeeds unconditionally; second, MUnbox has regalloc implications --
> we really want the data to be loaded as close to its uses as possible.

The idea would be attaching MUnbox and having a bit whether it needs a guard or not. We would also put the MUnbox on the stack instead of the MParameter, effectively making it the definition.

> We can skirt these issues by just expressing TI information via the MIRType
> of the MParameter, which makes everything just work. We never actually need
> explicit unboxing to make use of TI.

Yeah, definitely, we should do this anyway. We still need the unbox though since arguments are value-typed. You can't type a value without extracting the payload.

> This requirement implies that barriers are created by the users of an
> MDefinition, not at the point at which an MDefinition exists. Then a barrier
> exists iff the generated code depends on that invariant, and all barriers
> are at point-of-definition.

Barriers are different from guards. For example:

  function (a, b) {
    for (;;) {
      a();
      a = b.x;
    }
  }

"a" might have a type check (or an unbox, or both) in its prologue.
"b.x" may have a type barrier.
"a()" might need a callee guard, information we can't extract from MIR.
"a()" might produce a value which must go through a type barrier.

Barriers are distinct and must be placed whether or not a value is consumed as a specific type. However, sometimes we can infer that placing a guard will have the same effect as a barrier, or we may be able to eliminate guards as part of GVN or type analysis.

> 3. The initial trampoline that executes MParameter barrier checks should
> then be generated by iterating over the MParameters as the first step of
> codegen, which requires no modification, and doesn't *really* have to be a
> separate trampoline, unless it's easier to think of it that way. We can then
> insert a label, or some equivalent, to mark the absolute address at which
> these checks have finished; the address would be stored in the IonScript.

Okay, I think I see what you're saying w.r.t (1) now. I like this idea. It only works for parameters, but makes total sense. If I understand, our prologue would look like this:

    type_checks:
      cmp param1, blah
      jne _invalidate
      cmp param2, blah
      jne _invalidate
      ...
    fast_entry:
      sub $0x80, %esp
      unbox param1
      unbox param2

> Note that this should not require a separate frame to handle argument
> checking

At first I thought this wasn't true, but now looking at it, I think a bailout inside the type check block would work, as long as we make sure to use wide bailouts and not bailout tables (since scripts can only use one bailout table). The problem is that the type checks have a frame size of 0 and the rest of the script has the real frame size.
Comment 4 David Anderson [:dvander] 2011-09-26 18:38:29 PDT
Well, directly typing parameters is probably annoying. We'd have to change how parameters get lowered, which could be tricky. For example say a parameter defines a string, the register allocator will attempt to string load from its backing store, when in reality it needs to perform an unbox.

We really just want to throw infallible MUnbox instructions in the MIR, for each applicable parameter. But we can still use the pattern above where type checks are hand-generated before the method proper.
Comment 5 David Anderson [:dvander] 2011-09-27 00:02:56 PDT
Created attachment 562673 [details] [diff] [review]
wip v0

This doesn't compile yet, we need a bunch of new macro assembler abstractions to get there. However making arguments typed by replacing them with infallible MUnbox instructions does work. This patch also condenses all the x64 unbox instructions to one LIR which removes a lot of code.

This is the first scenario where I've felt the need to have something like branchPtr()/branchTest32() from Nitro, and it seems like the value-testing pattern of:
  cond = testType(cond, value);
  j(cond, &label);

isn't really ideal. So instead I'm proposing:
  testType(cond, value, &label);

And then adding the Nitro-esque functions like:
  branchPtr(cond, lhs, rhs, &label);
Comment 6 David Anderson [:dvander] 2011-10-06 18:52:22 PDT
Created attachment 565427 [details] [diff] [review]
wip v1

Getting back to this now - rebased, compiles. Just have to split the entry points.
Comment 7 Nicolas B. Pierron [:nbp] 2011-10-07 01:53:08 PDT
Comment on attachment 565427 [details] [diff] [review]
wip v1

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

::: js/src/ion/shared/Assembler-shared.h
@@ +80,5 @@
>  };
>  
> +// Specifies an address computed in the form of a register base and a constant,
> +// 32-bit offset.
> +struct Address

Isn't this structure defined in Bug 684402 Attachment 558711 [details] [diff] ?
Couldn't we have a separated issue for it such as I can reuse it for Bug 685228.
Comment 8 AWAY Tom Schuster [:evilpie] 2011-10-07 01:59:11 PDT
Comment on attachment 565427 [details] [diff] [review]
wip v1

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

::: js/src/ion/IonBuilder.cpp
@@ +227,5 @@
> +            param = MUnbox::NewUnchecked(param, MIRTypeFromValueType(definiteType));
> +        break;
> +    }
> +
> +    current->add(param);

I don't know if this matters or could happen, but wouldn't that add param twice if definiteType == JSVAL_TYPE_UNKNOWN?
Comment 9 David Anderson [:dvander] 2011-10-07 17:45:39 PDT
Created attachment 565696 [details] [diff] [review]
part 1: everything but ARM

This patch has a lot of minor refactorings and cleanups in it, the meat of it is:
 (1) After creating MStart, we apply TI and create infallible unbox operations on parameters, replacing them in the stack.
 (2) Methods begin with a fake-o frame checking their arguments. After checking is completed, the fake frame is removed. The type testing API is hardcoded for an Address right now but it's designed to be templated later on, for type barriers.

Once we have monomorphic calls, we'll be able to bypass the header of methods and go right to the fast part.
Comment 10 David Anderson [:dvander] 2011-10-07 17:47:03 PDT
Oh, and, LIR nodes now can always reach their MIR when added with standard define() functions.
Comment 11 David Anderson [:dvander] 2011-10-07 17:48:14 PDT
Comment on attachment 565696 [details] [diff] [review]
part 1: everything but ARM

Brian, could you double-check the TypeSet testing stuff in IonMacroAssembler.cpp?
Comment 12 David Anderson [:dvander] 2011-10-10 00:26:31 PDT
(In reply to Nicolas B. Pierron [:pierron] from comment #7)
> ::: js/src/ion/shared/Assembler-shared.h
> Isn't this structure defined in Bug 684402 Attachment 558711 [details] [diff]
> [details] ?
> Couldn't we have a separated issue for it such as I can reuse it for Bug
> 685228.

Sorry my queue fell behind - if you need this now, the struct is really small. Feel free to tear it out and use it in your patch. Hopefully this will land soon.
Comment 13 Brian Hackett (:bhackett) 2011-10-10 06:07:02 PDT
Comment on attachment 565696 [details] [diff] [review]
part 1: everything but ARM

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

Looks good.  FWIW there are two upcoming changes that will affect type set testing.  Bug 678687 (want to land for Fx10) will allow coercing doubles into integers when resolving type barriers, and object shrinking will kill the SINGLETON_TYPE and LAZY_TYPE flags (though the changes here will actually be a speed improvement).
Comment 14 Sean Stangl [:sstangl] 2011-10-10 14:40:58 PDT
Comment on attachment 565696 [details] [diff] [review]
part 1: everything but ARM

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

This patch seems generally good. The distinction between the parameter validation phase and function code eliminates some optimization opportunities, but simplifies what would otherwise be complicated entry and unbox logic.

::: js/src/ion/CodeGenerator.cpp
@@ +245,5 @@
> +
> +    // Reserve the amount of stack the actual frame will use. We have to undo
> +    // this before falling through to the method proper though, because the
> +    // monomorphic call case will bypass this entire path.
> +    masm.reserveStack(frameSize());

This is actually pretty nice.

::: js/src/ion/IonLIR.h
@@ +604,5 @@
>          return snapshot_;
>      }
> +    void setMir(MDefinition *mir) {
> +        mir_ = mir;
> +    }

:)

::: js/src/ion/IonMacroAssembler.cpp
@@ +135,5 @@
> +    Label matched;
> +    Register tag = extractTag(address, scratch);
> +
> +    if (types->hasType(types::Type::DoubleType())) {
> +        // Type sets containing double also contain int.

Can we assert in place of the comment?

::: js/src/ion/IonMacroAssembler.h
@@ +144,5 @@
>      void callWithABI(void *fun);
> +
> +    // Emits a test of a value against all types in a TypeSet. A scratch
> +    // register is required.
> +    void testTypeSet(const Address &address, types::TypeSet *types, Register scratch0,

nit: s/0//

Also, testFoo() in our assembler generally implies a Condition return type; this function signature looks more like a branch. Maybe "guardTypeSet"?

::: js/src/ion/MIR.h
@@ +1027,5 @@
> +    {
> +        return new MUnbox(ins, type, false);
> +    }
> +
> +    bool checkType() const {

This sounds like a function that checks a type, instead of an accessor for checkType_(). "mustCheckType()"?

::: js/src/ion/MIRGraph.cpp
@@ +285,1 @@
>          // In this case, we want the second assignment to act as though there

First line of comment is cut off.

::: js/src/ion/x64/Lowering-x64.cpp
@@ +119,5 @@
>  LIRGeneratorX64::assignSnapshot(LInstruction *ins)
>  {
>      LSnapshot *snapshot = LSnapshot::New(gen, lastResumePoint_);
>      if (!snapshot)
> +        return NULL;

Should still be |false|.

::: js/src/ion/x64/MacroAssembler-x64.h
@@ +205,5 @@
>      }
>  
> +    void branchPtr(Condition cond, Register lhs, ImmGCPtr ptr, Label *label) {
> +        movq(ptr, ScratchReg);
> +        cmpq(lhs, ScratchReg);

Express in terms of cmpPtr()?

@@ +232,5 @@
>  
>      void splitTag(const ValueOperand &operand, const Register &dest) {
> +        if (operand.valueReg() != dest)
> +            movq(operand.valueReg(), dest);
> +        shrq(Imm32(JSVAL_TAG_SHIFT), dest);

If operand.valueReg() == dest, and we perform shrq on dest, then we should unset operand.valueReg(), as it no longer holds a value.

Permitting this function to mutate the valueReg seems a likely source of future bugs.

::: js/src/ion/x86/MacroAssembler-x86.h
@@ +293,5 @@
>      }
>  
> +    // Extended unboxing API. If the payload is already in a register, returns
> +    // that register. Otherwise, provides a move to the given scratch register,
> +    // and returns that.

Is this comment accurate? I do not see logic that checks whether the payload is already in a register.

::: js/src/jsval.h
@@ +286,5 @@
>  #define JSVAL_UPPER_EXCL_SHIFTED_TAG_OF_NUMBER_SET       JSVAL_SHIFTED_TAG_UNDEFINED
>  #define JSVAL_LOWER_INCL_SHIFTED_TAG_OF_PTR_PAYLOAD_SET  JSVAL_SHIFTED_TAG_MAGIC
>  #define JSVAL_LOWER_INCL_SHIFTED_TAG_OF_GCTHING_SET      JSVAL_SHIFTED_TAG_STRING
>  
> +#define JSVAL_UPPER_INCL_TAG_OF_NUMBER_SET              JSVAL_TAG_INT32

These defines already exist above, under "#if JS_BITS_PER_WORD == 32". Should we just include these tag defines in either case?
Comment 15 David Anderson [:dvander] 2011-10-10 18:22:04 PDT
> This sounds like a function that checks a type, instead of an accessor for
> checkType_(). "mustCheckType()"?

Yeah this name is bad. It gets changed in the GETGNAME patch.

> @@ +232,5 @@
> >  
> >      void splitTag(const ValueOperand &operand, const Register &dest) {
> > +        if (operand.valueReg() != dest)
> > +            movq(operand.valueReg(), dest);
> > +        shrq(Imm32(JSVAL_TAG_SHIFT), dest);
> 
> If operand.valueReg() == dest, and we perform shrq on dest, then we should
> unset operand.valueReg(), as it no longer holds a value.

The API here isn't the best. I'll create another function which takes in two regs, and make the ValueOperand one assert src != dest. The ValueOperand one is always called with ScratchReg.

> Is this comment accurate? I do not see logic that checks whether the payload
> is already in a register.

It's true after the GETGNAME patch, where more overloads become available.

> 
> ::: js/src/jsval.h
> @@ +286,5 @@
> >  #define JSVAL_UPPER_EXCL_SHIFTED_TAG_OF_NUMBER_SET       JSVAL_SHIFTED_TAG_UNDEFINED
> >  #define JSVAL_LOWER_INCL_SHIFTED_TAG_OF_PTR_PAYLOAD_SET  JSVAL_SHIFTED_TAG_MAGIC
> >  #define JSVAL_LOWER_INCL_SHIFTED_TAG_OF_GCTHING_SET      JSVAL_SHIFTED_TAG_STRING
> >  
> > +#define JSVAL_UPPER_INCL_TAG_OF_NUMBER_SET              JSVAL_TAG_INT32
> 
> These defines already exist above, under "#if JS_BITS_PER_WORD == 32".
> Should we just include these tag defines in either case?

I think the intent is to have "tags" be platform-specific and the generic area is for "types".

http://hg.mozilla.org/projects/ionmonkey/rev/e3463b292ab4 w/ nits picked

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