Last Comment Bug 707423 - IonMonkey: compile global scripts
: IonMonkey: compile global scripts
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: David Anderson [:dvander]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 776361
Blocks: IonSpeed
  Show dependency treegraph
 
Reported: 2011-12-03 02:59 PST by Jan de Mooij [:jandem]
Modified: 2012-07-22 09:00 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP v0, runs bitwise-and (13.56 KB, patch)
2012-01-10 18:59 PST, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
WIP v1, handles bailouts (19.99 KB, patch)
2012-01-10 19:59 PST, David Anderson [:dvander]
no flags Details | Diff | Splinter Review
patch (21.69 KB, patch)
2012-01-10 20:54 PST, David Anderson [:dvander]
cdleary: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2011-12-03 02:59:58 PST
We need this for bitwise-and.
Comment 1 David Anderson [:dvander] 2012-01-10 18:59:36 PST
Created attachment 587571 [details] [diff] [review]
WIP v0, runs bitwise-and
Comment 2 David Anderson [:dvander] 2012-01-10 19:59:55 PST
Created attachment 587581 [details] [diff] [review]
WIP v1, handles bailouts
Comment 3 David Anderson [:dvander] 2012-01-10 20:54:05 PST
Created attachment 587585 [details] [diff] [review]
patch

passes tests
Comment 4 Chris Leary [:cdleary] (not checking bugmail) 2012-01-11 11:41:36 PST
Comment on attachment 587585 [details] [diff] [review]
patch

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

Just a few nits. Looks good!

::: js/src/ion/Bailouts.cpp
@@ +259,5 @@
>          return BAILOUT_RETURN_FATAL_ERROR;
>      activation->setBailout(br);
>  
>      // Non-function frames are not supported yet. We don't compile or enter
>      // global scripts so this assert should not fire yet.

Nit: can remove this comment, because we do!

::: js/src/ion/CompileInfo.h
@@ +46,5 @@
>  
>  inline uintN
>  CountArgSlots(JSFunction *fun)
>  {
> +    return fun ? fun->nargs + 2 : 1; // +2 for |scopeChain| and |this|, or +1 for |this|

As discussed on IRC, I think the global object gets +1 for |scopeChain| instead of |this|

::: js/src/ion/IonBuilder.cpp
@@ +2444,5 @@
>          osrBlock->add(scopev);
>          osrBlock->initSlot(slot, scopev);
>      }
>  
>      // Initialize |this| parameter.

Nit: can we push this comment down into the block?

::: js/src/jit-test/tests/v8-v5/check-splay.js
@@ +91,5 @@
>    } while (splayTree.find(key) != null);
>    splayTree.insert(key, GeneratePayloadTree(kSplayTreePayloadDepth, key));
>    return key;
>  }
> +dis(InsertNewNode);

Nit: not sure if it's bad to leave this in.
Comment 5 David Anderson [:dvander] 2012-01-11 15:39:29 PST
http://hg.mozilla.org/projects/ionmonkey/rev/f3fef5d48874

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