Last Comment Bug 714397 - IonMonkey: Cannot inline functions with too many provided arguments.
: IonMonkey: Cannot inline functions with too many provided arguments.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 12 Branch
: All All
: -- normal (vote)
: ---
Assigned To: Chris Leary [:cdleary] (not checking bugmail)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-30 17:56 PST by Sean Stangl [:sstangl]
Modified: 2012-01-04 14:42 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test case, reduced from crypto-md5. (172 bytes, application/javascript)
2011-12-30 17:56 PST, Sean Stangl [:sstangl]
no flags Details
Pad to nargs if necessary. (2.92 KB, patch)
2012-01-04 11:56 PST, Chris Leary [:cdleary] (not checking bugmail)
sstangl: review+
Details | Diff | Splinter Review

Description Sean Stangl [:sstangl] 2011-12-30 17:56:48 PST
Created attachment 585090 [details]
Test case, reduced from crypto-md5.

The attached test case asserts with --ion -n:
> Assertion failure: args.length() == info().nargs(), at /home/sstangl/dev/ionmonkey/js/src/ion/IonBuilder.cpp:290

It looks like inlining does not yet handle the case where too many arguments are provided.
Comment 1 Chris Leary [:cdleary] (not checking bugmail) 2012-01-04 11:56:21 PST
Created attachment 585838 [details] [diff] [review]
Pad to nargs if necessary.
Comment 2 Sean Stangl [:sstangl] 2012-01-04 13:59:48 PST
Comment on attachment 585838 [details] [diff] [review]
Pad to nargs if necessary.

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

::: js/src/ion/IonBuilder.cpp
@@ +295,5 @@
> +    const size_t nargs = info().nargs();
> +    if (args.length() < nargs) {
> +        for (size_t i = 0, missing = nargs - args.length(); i < missing; ++i) {
> +            MConstant *undef = MConstant::New(UndefinedValue());
> +            current->add(undef);

Do we have to add so many distinct constants? Can we just add one outside of the for() loop, then append it multiple times to args? GVN will wind up undoing the work here, but we can avoid it early.
Comment 3 David Anderson [:dvander] 2012-01-04 14:02:27 PST
(In reply to Sean Stangl from comment #2)
> Do we have to add so many distinct constants? Can we just add one outside of
> the for() loop, then append it multiple times to args? GVN will wind up
> undoing the work here, but we can avoid it early.

We can't constant propagate until SSA is finished (see the big comment above MBasicBlock::setVariable - when rewriting a loop we could accidentally overwrite uses of the wrong argument).
Comment 4 Chris Leary [:cdleary] (not checking bugmail) 2012-01-04 14:42:59 PST
https://hg.mozilla.org/projects/ionmonkey/rev/8d6cfd44818f

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