670816 - IonMonkey: Run generated code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

Take the output of the code generator, put it into executable memory, and run it. To do this we need to attach some sort of information to JSScripts and put a hook in the interpreter to run Ion code. This also wants some sort of shell flag so we can disable IonMonkey by default.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, data structures

Comment 2

[Andrew Drake [:adrake]]

Comment on attachment 546048 [details] [diff] [review]
part 1, data structures

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

::: js/src/jsscript.h
@@ +379,5 @@
>  namespace mjit {
> +    struct JITScript;
> +}
> +namespace ion {
> +    struct IonScript;

Does this want to be #ifdef ION?

@@ +529,5 @@
>      /* array of execution counters for every JSOp in the script, by runmode */
>      JSPCCounters    pcCounters;
>  
>    public:
> +    js::ion::IonScript *ion;          /* Information attached by Ion */

Or this?

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Pushed part 1 with nit@ http://hg.mozilla.org/users/danderson_mozilla.com/ionmonkey/rev/4ec0e4e86693

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2: shell flags!

This patch adds shell flags and disables Ion by default. The interpreter hookup will come next. Patch needs bug some uncommitted stuff from 668095 which hopefully can land soon.

Shell flags:
  --ion (off by default)
  --ion-licm=on/off (on default)
  --ion-lsra=on/off (off default, ssa builder has a bug)
  --ion-gvn=off/pessimistic/optimistic

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3: interp changes

This patch adds interpreter hooks for compiling and entering into Ion code at method entry points.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4: use gc to auto decref code objects

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4.1: use gc to auto decref code objects

Whoops, forgot to trace IonScripts through JSScript

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4.2: use gc to auto decref code objects

Fixes bugs Bill pointed out in person, the marking was basically wrong.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 546240 [details] [diff] [review]
part 4.2: use gc to auto decref code objects

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

I realized that there's some subtlety here. Namely, do we want to expose Ion code objects to the cycle collector? If they can reference JS objects, then we ought to. Otherwise we shouldn't. The current code is sort of exposing Ion code objects: when Mark<IonCode> is called, it calls the trace callback, which may be the cycle collector. A quick hack to avoid this is to test IS_GC_MARKING_TRACER(trc) in MarkIonCode. If it's false, then just return without doing any work. If you *do* want to involve the cycle collector, then you need to implement a case in JS_TraceChildren.

::: js/src/jscntxt.h
@@ +401,5 @@
>      void                *gcMarkStackObjs[js::OBJECT_MARK_STACK_SIZE / sizeof(void *)];
>      void                *gcMarkStackRopes[js::ROPES_MARK_STACK_SIZE / sizeof(void *)];
>      void                *gcMarkStackXMLs[js::XML_MARK_STACK_SIZE / sizeof(void *)];
>      void                *gcMarkStackLarges[js::LARGE_MARK_STACK_SIZE / sizeof(void *)];
> +    void                *gcMarkStackIonCode[js::LARGE_MARK_STACK_SIZE / sizeof(void *)];

You might want a separate size constant, but I'll leave that up to you.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4.3: use gc to auto decref code objects

At the moment I'm not sure whether or not we should involve the cycle collector, so I'll err on the safe side and add the hook. We can remove it later if it's unnecessary.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 546253 [details] [diff] [review]
part 4.3: use gc to auto decref code objects

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

Comment 12

[Andrew Drake [:adrake]]

Comment on attachment 546081 [details] [diff] [review]
part 2: shell flags!

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

::: js/src/ion/Ion.cpp
@@ +198,5 @@
>      if (!lirgen.generate())
>          return false;
>      spew.spewPass("Generate LIR");
>  
> +    if (!js_IonOptions.lsra) {

Nit: Can we avoid if (!foo) { } else { } ?

::: js/src/shell/js.cpp
@@ +5638,5 @@
>  
>  static int
> +OptionFailure(const char *option, const char *str)
> +{
> +    fprintf(stderr, "Recognized option for %s: %s\n", option, str);

Typo "Unrecognized"

@@ +5974,5 @@
> +                               "  pessimistic: use pessimistic GVN\n"
> +                               "  optimistic: (default) use optimistic GVN")
> +        || !op.addStringOption('\0', "ion-licm", "on/off",
> +                               "Loop invariant code motion (default: on, off to disable)")
> +        || !op.addStringOption('\0', "ion-lsra", "on/off",

Would ion-regalloc=lsra/greedy be objectionable? It strikes me as more of an either/or than an on/off.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

> > +    if (!js_IonOptions.lsra) {
> 
> Nit: Can we avoid if (!foo) { } else { } ?

Definitely.

> Would ion-regalloc=lsra/greedy be objectionable? It strikes me as more of an
> either/or than an on/off.

I like this better as well.

Comment 14

[Ryan Pearl [:rpearl]]

Comment on attachment 546081 [details] [diff] [review]
part 2: shell flags!

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

Looks good, a few things:

::: js/src/ion/Ion.cpp
@@ +179,5 @@
>          return false;
>      spew.spewPass("Apply types");
>  
> +    if (js_IonOptions.gvn) {
> +        ValueNumberer gvn(graph, !js_IonOptions.gvnIsOptimistic);

Would it be better to change the parameter to ValueNumberer to be optimistic, instead of pessimistic, to match this flag?

::: js/src/ion/Ion.h
@@ -53,0 +53,16 @@
> > +struct IonOptions
> > +{
> > +    // If Ion is supported, this toggles whether Ion is used.
> > +    //
NaN more ...

Here you comment that the default is false (pessimistic), but in the constructor and usage string it is initialized to true (optimistic).

::: js/src/shell/js.cpp
@@ +5638,5 @@
>  
>  static int
> +OptionFailure(const char *option, const char *str)
> +{
> +    fprintf(stderr, "Recognized option for %s: %s\n", option, str);

I think you mean "Unrecognized"?

Comment 15

[Ryan Pearl [:rpearl]]

Apparently adrake and I had a review race condition. But, one of my comments got confused (or I did? whoops...)

> Here you comment that the default is false (pessimistic), but in the
> constructor and usage string it is initialized to true (optimistic).
> 

This should refer to js/src/ion/Ion.h:65-69

Comment 16

[Andrew Drake [:adrake]]

Comment on attachment 546095 [details] [diff] [review]
part 3: interp changes

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

::: js/src/ion/Ion.cpp
@@ +238,5 @@
>      return true;
>  }
>  
> +MethodStatus
> +ion::Compile(JSContext *cx, JSScript *script, js::StackFrame *fp)

ion::IonCompile vs ion::Compile seems like it could get confusing, but I'm not sure of a better name. I'm tempted to say just merge them, since I don't see IonCompile getting called from anywhere but here.

::: js/src/ion/Ion.h
@@ +92,5 @@
> +
> +enum MethodStatus
> +{
> +    Method_CantCompile,
> +    Method_Compiled

Are there going to be any return statuses other than these two?

Comment 17

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 5: manage ion compartment memory

This patch refactors IonCompartment and code linking a little so memory is managed right. We're still not marking the IonCompartment though, that's the next patch.

Comment 18

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 6: mark compartments

Bill, would you be able to check whether my compartment marking here is okay?

After this patch, Ion code memory should be fully managed.

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 6: mark compartments, actual patch

Comment 20

[Andrew Scheff]

Comment on attachment 546658 [details] [diff] [review]
part 5: manage ion compartment memory

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

Cool. Certainly is cleaner. Looks fine to me.

This is applied on top of my IonCompartments patch... should I push that? Or you will do it?

Comment 21

[David Anderson [:dvander] - inactive, e-mail if emergency]

I'll push it, it's in my queue -- one more to go.

Comment 22

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 7: run

Run code.

Comment 23

[David Anderson [:dvander] - inactive, e-mail if emergency]

p2: http://hg.mozilla.org/users/danderson_mozilla.com/ionmonkey/rev/8db8eef79b8c
p3: http://hg.mozilla.org/users/danderson_mozilla.com/ionmonkey/rev/73f673243e1f
p4: http://hg.mozilla.org/users/danderson_mozilla.com/ionmonkey/rev/179176fe560b
p5: http://hg.mozilla.org/users/danderson_mozilla.com/ionmonkey/rev/fcb18ae229c3

Comment 24

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 6: mark compartments, weak refs

Bill pointed out that having strong refs in the compartment would keep it alive forever. This version uses weak refs when the compartment is not active.

Comment 25

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 7: run, align stack in trampoline

Okay, the trampolines worked perfectly but I hadn't accounted for two things:
 (1) Stack alignment.
 (2) JIT code clobbering EBP.

So I've rejiggered the trampoline to align the stack. The new calling convention is:
   [padding]
   [args...]
   <numBytesPushed>
   <returnAddress>

For example if we push 2 arguments and the stack needed 4 bytes of alignment:
   <padding>
   <arg1.type>
   <arg1.data>
   <arg0.type>
   <arg0.data>
   2*sizeof(Value) + sizeof(padding)
   <return address>

This way when the JIT code returns, we can figure out how much stack to adjust by popping once, and from there can recover ebp or other frame variables.
   
x64 will need something similar, but there we have a guarantee that at the start of the trampoline the stack is 16-byte aligned. This guarantee was not universal on x86.

Comment 26

[Andrew Scheff]

Comment on attachment 546703 [details] [diff] [review]
part 7: run, align stack in trampoline

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

Ok cool.  I'll implement this on x64!

Comment 27

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 546691 [details] [diff] [review]
part 6: mark compartments, weak refs

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

There's a slight problem here, I think. We don't actually set the active flag of a compartment until part way through MarkRuntime. (I think it's done when the thread data is marked.) I would recommend moving the compartment marking into MarkRuntime, right before the extra roots trace op. And you should be able to call it in this way:
  PER_COMPARTMENT_OP(rt, c->mark(trc));
That will take care of choosing which compartments to consider.

Otherwise looks good. r+ with that fixed.

Comment 28

[Andrew Drake [:adrake]]

Comment on attachment 546703 [details] [diff] [review]
part 7: run, align stack in trampoline

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

::: js/src/ion/x86/Architecture-x86.h
@@ +59,5 @@
> +// In bytes: slots needed for potential memory->memory move spills.
> +//   +8 for cycles
> +//   +4 for gpr spills
> +//   +4 for double spills
> +static const uint32 ION_FRAME_SLACK_SIZE     = 20;

I think you mean +8 for double spills?

Comment 29

[Nicolas B. Pierron [:nbp]]

Comment on attachment 546048 [details] [diff] [review]
part 1, data structures

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

::: js/src/jsscript.h
@@ +379,5 @@
>  namespace mjit {
> +    struct JITScript;
> +}
> +namespace ion {
> +    struct IonScript;

In fact you want to move it inside a #ifdef JS_ION otherwise the compilation does not work with --disable-methodjit.

Comment 30

[Nicolas B. Pierron [:nbp]]

Attachment: Fix part1.

Comment 31

[Nicolas B. Pierron [:nbp]]

Reopen the bug for part1 related fix.