IonMonkey: Run generated code

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 6 obsolete attachments)

7.58 KB, patch
adrake
: review+
Details | Diff | Splinter Review
10.06 KB, patch
adrake
: review+
Details | Diff | Splinter Review
4.86 KB, patch
adrake
: review+
Details | Diff | Splinter Review
24.12 KB, patch
billm
: review+
Details | Diff | Splinter Review
9.02 KB, patch
Andrew Scheff
: review+
Details | Diff | Splinter Review
4.89 KB, patch
billm
: review+
Details | Diff | Splinter Review
27.21 KB, patch
adrake
: review+
Andrew Scheff
: review+
Details | Diff | Splinter Review
576 bytes, patch
dvander
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Updated

6 years ago
Depends on: 670819
(Assignee)

Updated

6 years ago
Depends on: 670820
(Assignee)

Comment 1

6 years ago
Created attachment 546048 [details] [diff] [review]
part 1, data structures
Assignee: general → dvander
Status: NEW → ASSIGNED
Attachment #546048 - Flags: review?(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?
Attachment #546048 - Flags: review?(adrake) → review+
(Assignee)

Comment 3

6 years ago
Pushed part 1 with nit@ http://hg.mozilla.org/users/danderson_mozilla.com/ionmonkey/rev/4ec0e4e86693
(Assignee)

Comment 4

6 years ago
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
Attachment #546081 - Flags: review?(adrake)
(Assignee)

Comment 5

6 years ago
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.
Attachment #546095 - Flags: review?(adrake)
(Assignee)

Comment 6

6 years ago
Created attachment 546230 [details] [diff] [review]
part 4: use gc to auto decref code objects
Attachment #546230 - Flags: review?(wmccloskey)
(Assignee)

Comment 7

6 years ago
Created attachment 546232 [details] [diff] [review]
part 4.1: use gc to auto decref code objects

Whoops, forgot to trace IonScripts through JSScript
Attachment #546230 - Attachment is obsolete: true
Attachment #546232 - Flags: review?(wmccloskey)
Attachment #546230 - Flags: review?(wmccloskey)
(Assignee)

Comment 8

6 years ago
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.
Attachment #546232 - Attachment is obsolete: true
Attachment #546240 - Flags: review?(wmccloskey)
Attachment #546232 - Flags: review?(wmccloskey)
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.
(Assignee)

Comment 10

6 years ago
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.
Attachment #546240 - Attachment is obsolete: true
Attachment #546253 - Flags: review?(wmccloskey)
Attachment #546240 - Flags: review?(wmccloskey)
Comment on attachment 546253 [details] [diff] [review]
part 4.3: use gc to auto decref code objects

Review of attachment 546253 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #546253 - Flags: review?(wmccloskey) → review+
(Assignee)

Updated

6 years ago
Attachment #546081 - Flags: review?(adrake) → review?(rpearl)
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.
Attachment #546081 - Flags: review?(rpearl) → review+
(Assignee)

Comment 13

6 years ago
> > +    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 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"?
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 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?
Attachment #546095 - Flags: review?(adrake) → review+

Updated

6 years ago
Depends on: 672377
(Assignee)

Comment 17

6 years ago
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.
Attachment #546658 - Flags: review?(ascheff)
(Assignee)

Comment 18

6 years ago
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.
Attachment #546659 - Flags: review?(wmccloskey)
(Assignee)

Comment 19

6 years ago
Created attachment 546660 [details] [diff] [review]
part 6: mark compartments, actual patch
Attachment #546659 - Attachment is obsolete: true
Attachment #546660 - Flags: review?(wmccloskey)
Attachment #546659 - Flags: review?(wmccloskey)

Updated

6 years ago
No longer depends on: 672377

Comment 20

6 years ago
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?
Attachment #546658 - Flags: review?(ascheff) → review+
(Assignee)

Comment 21

6 years ago
I'll push it, it's in my queue -- one more to go.
(Assignee)

Comment 22

6 years ago
Created attachment 546686 [details] [diff] [review]
part 7: run

Run code.
Attachment #546686 - Flags: review?(ascheff)
(Assignee)

Updated

6 years ago
Attachment #546686 - Flags: review?(adrake)
(Assignee)

Comment 23

6 years ago
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
(Assignee)

Comment 24

6 years ago
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.
Attachment #546660 - Attachment is obsolete: true
Attachment #546691 - Flags: review?(wmccloskey)
Attachment #546660 - Flags: review?(wmccloskey)
(Assignee)

Comment 25

6 years ago
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.
Attachment #546686 - Attachment is obsolete: true
Attachment #546703 - Flags: review?(adrake)
Attachment #546686 - Flags: review?(ascheff)
Attachment #546686 - Flags: review?(adrake)
(Assignee)

Updated

6 years ago
Attachment #546703 - Flags: review?(ascheff)

Comment 26

6 years ago
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!
Attachment #546703 - Flags: review?(ascheff) → review+
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.
Attachment #546691 - Flags: review?(wmccloskey) → review+
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?
Attachment #546703 - Flags: review?(adrake) → review+
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
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.
Created attachment 564549 [details] [diff] [review]
Fix part1.
Attachment #564549 - Flags: review?(dvander)
Reopen the bug for part1 related fix.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

6 years ago
Attachment #564549 - Flags: review?(dvander) → review+
(Assignee)

Updated

6 years ago
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Depends on: 827436
You need to log in before you can comment on or make changes to this bug.