Last Comment Bug 670816 - IonMonkey: Run generated code
: IonMonkey: Run generated code
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: 670819 670820 827436
Blocks: IonMonkey
  Show dependency treegraph
 
Reported: 2011-07-11 19:11 PDT by David Anderson [:dvander]
Modified: 2013-01-07 10:57 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1, data structures (7.58 KB, patch)
2011-07-14 17:17 PDT, David Anderson [:dvander]
adrake: review+
Details | Diff | Splinter Review
part 2: shell flags! (10.06 KB, patch)
2011-07-14 20:06 PDT, David Anderson [:dvander]
adrake: review+
Details | Diff | Splinter Review
part 3: interp changes (4.86 KB, patch)
2011-07-14 22:05 PDT, David Anderson [:dvander]
adrake: review+
Details | Diff | Splinter Review
part 4: use gc to auto decref code objects (15.98 KB, patch)
2011-07-15 14:38 PDT, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
part 4.1: use gc to auto decref code objects (17.82 KB, patch)
2011-07-15 14:47 PDT, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
part 4.2: use gc to auto decref code objects (22.99 KB, patch)
2011-07-15 15:18 PDT, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
part 4.3: use gc to auto decref code objects (24.12 KB, patch)
2011-07-15 16:27 PDT, David Anderson [:dvander]
wmccloskey: review+
Details | Diff | Splinter Review
part 5: manage ion compartment memory (9.02 KB, patch)
2011-07-18 16:03 PDT, David Anderson [:dvander]
ascheff: review+
Details | Diff | Splinter Review
part 6: mark compartments (1.73 KB, patch)
2011-07-18 16:09 PDT, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
part 6: mark compartments, actual patch (3.65 KB, patch)
2011-07-18 16:09 PDT, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
part 7: run (20.14 KB, patch)
2011-07-18 17:35 PDT, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
part 6: mark compartments, weak refs (4.89 KB, patch)
2011-07-18 18:38 PDT, David Anderson [:dvander]
wmccloskey: review+
Details | Diff | Splinter Review
part 7: run, align stack in trampoline (27.21 KB, patch)
2011-07-18 20:18 PDT, David Anderson [:dvander]
adrake: review+
ascheff: review+
Details | Diff | Splinter Review
Fix part1. (576 bytes, patch)
2011-10-04 07:49 PDT, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review

Description David Anderson [:dvander] 2011-07-11 19:11:36 PDT
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] 2011-07-14 17:17:01 PDT
Created attachment 546048 [details] [diff] [review]
part 1, data structures
Comment 2 Andrew Drake [:adrake] 2011-07-14 17:22:16 PDT
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] 2011-07-14 17:35:42 PDT
Pushed part 1 with nit@ http://hg.mozilla.org/users/danderson_mozilla.com/ionmonkey/rev/4ec0e4e86693
Comment 4 David Anderson [:dvander] 2011-07-14 20:06:00 PDT
Created attachment 546081 [details] [diff] [review]
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] 2011-07-14 22:05:01 PDT
Created attachment 546095 [details] [diff] [review]
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] 2011-07-15 14:38:41 PDT
Created attachment 546230 [details] [diff] [review]
part 4: use gc to auto decref code objects
Comment 7 David Anderson [:dvander] 2011-07-15 14:47:54 PDT
Created attachment 546232 [details] [diff] [review]
part 4.1: use gc to auto decref code objects

Whoops, forgot to trace IonScripts through JSScript
Comment 8 David Anderson [:dvander] 2011-07-15 15:18:23 PDT
Created attachment 546240 [details] [diff] [review]
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 (:billm) 2011-07-15 15:44:27 PDT
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] 2011-07-15 16:27:42 PDT
Created attachment 546253 [details] [diff] [review]
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 (:billm) 2011-07-15 16:29:21 PDT
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] 2011-07-15 17:47:46 PDT
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] 2011-07-15 17:53:35 PDT
> > +    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] 2011-07-15 18:00:01 PDT
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] 2011-07-15 18:06:21 PDT
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] 2011-07-18 07:10:10 PDT
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] 2011-07-18 16:03:25 PDT
Created attachment 546658 [details] [diff] [review]
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] 2011-07-18 16:09:12 PDT
Created attachment 546659 [details] [diff] [review]
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] 2011-07-18 16:09:51 PDT
Created attachment 546660 [details] [diff] [review]
part 6: mark compartments, actual patch
Comment 20 Andrew Scheff 2011-07-18 16:21:12 PDT
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] 2011-07-18 16:40:30 PDT
I'll push it, it's in my queue -- one more to go.
Comment 22 David Anderson [:dvander] 2011-07-18 17:35:34 PDT
Created attachment 546686 [details] [diff] [review]
part 7: run

Run code.
Comment 24 David Anderson [:dvander] 2011-07-18 18:38:46 PDT
Created attachment 546691 [details] [diff] [review]
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] 2011-07-18 20:18:59 PDT
Created attachment 546703 [details] [diff] [review]
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 2011-07-19 10:45:02 PDT
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 (:billm) 2011-07-19 11:51:31 PDT
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] 2011-07-19 16:20:40 PDT
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] 2011-10-04 07:44:50 PDT
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] 2011-10-04 07:49:34 PDT
Created attachment 564549 [details] [diff] [review]
Fix part1.
Comment 31 Nicolas B. Pierron [:nbp] 2011-10-04 07:51:16 PDT
Reopen the bug for part1 related fix.

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