714397 - IonMonkey: Cannot inline functions with too many provided arguments.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: 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)]

Attachment: Pad to nargs if necessary.

Comment 2

[Sean Stangl [:sstangl]]

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] - inactive, e-mail if emergency]

(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)]

https://hg.mozilla.org/projects/ionmonkey/rev/8d6cfd44818f