1478350 - The MSetFrameArgument case in IonBuilder::jsop_setarg seems to be dead code

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[André Bargull [:anba]]

While investigating bug 1476417, I wondered if a similar issue exists for MSetFrameArgument, but the MSetFrameArgument code [1] is never executed when running jit-tests with --tbpl and code-coverage [2] agrees that it is never run. 

If it is possible to trigger this code, we should add a regression test for it. If it's not or no longer possible to run the code, we should simply remove it.

[1] https://searchfox.org/mozilla-central/rev/bdfd20ef30d521b57d5b6feeda71325e8b4cad66/js/src/jit/IonBuilder.cpp#12270-12284
[2] https://codecov.io/gh/marco-c/gecko-dev/src/master/js/src/jit/IonBuilder.cpp

Comment 1

[André Bargull [:anba]]

Attachment: Bug 1478350 - Part 1: Move JSOp::GetArg handling into a separate function. r=jandem!

Add IonBuilder::jsop_getarg next to the existing IonBuilder::jsop_setarg, so
we have both operations next to each other, which makes it easier to compare
how aliased arguments are handled.

Comment 2

[André Bargull [:anba]]

Attachment: Bug 1478350 - Part 2: Simplify return statement in IonBuilder::jsop_setarg. r=jandem!

Drive-by change: GET_ARGNO(pc) is passed to the function as arg, so we
don't need to recompute it again within the function.

Depends on D64567

Comment 3

[André Bargull [:anba]]

Attachment: Bug 1478350 - Part 3: Restructure IonBuilder::jsop_setarg. r=jandem!

Restructure jsop_setarg() to make it clearer when we abort Ion compilation.

The MSetFrameArgument code added in bug 921120 (and later disabled in bug 957475),
was written before bug 995336, so it did handle a case which can no longer happen,
namely JSOp::SetArg bytecode operations on lazy arguments. If we want to support
the use case from bug 921120, we will need to update the arguments analysis first.

The restructuring happened as follows:

info().argsObjAliasesFormals() and info().argumentsAliasesFormals() both
imply info().hasArguments(), so when we handle !info().hasArguments() first,
we just need to emit current->setArg(arg) and can then directly return.

Furthermore info().argsObjAliasesFormals() and info().argumentsAliasesFormals()
also imply mapped arguments semantics, so we handle !info().hasMappedArgsObj()
next and abort the Ion compilation. This is just done to match the current
behaviour and should be removed in a follow-up patch!

When we're running the arguments analysis, info().needsArgsObj() is true, so
we always end up with emitting MSetArgumentsObjectArg. The arguments analysis
will then flag the arguments object as being needed, cf. ArgumentsUseCanBeLazy().
That means info().needsArgsObj() is always true at this point, whether or not
we're currently running the arguments analysis.

Depends on D64568

Comment 4

[André Bargull [:anba]]

Attachment: Bug 1478350 - Part 4: Remove no longer used MSetFrameArgument node. r=jandem!

Part 3 removed the only place where MSetFrameArgument instructions were created.

Depends on D64569

Comment 5

[André Bargull [:anba]]

Attachment: Bug 1478350 - Part 5: Remove an outdated comment in IonBuilder::jsop_setarg. r=jandem!

This comment dates back to before bug 921120 and bug 995336 were written and no
longer makes any sense with the current implementation.

Depends on D64570

Comment 6

[André Bargull [:anba]]

Attachment: Bug 1478350 - Part 6: Remove AliasSet::FrameArgument. r=jandem!

We don't create lazy arguments when JSOp::SetArg is present, which means
modifiesArguments() is always false in this method.

Depends on D64571

Comment 7

[André Bargull [:anba]]

Attachment: Bug 1478350 - Part 7: Update a method name in a comment. r=jandem!

Looks like this should be unaliasedActual(), when interpreting this changeset
correctly:
https://hg.mozilla.org/mozilla-central/rev/adcd5d3c984efef3688189b11c6ec6276ad014c8

Drive-by change: Remove a stray, commented out code line.

Depends on D64572

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0b1bbf5387c
Part 1: Move JSOp::GetArg handling into a separate function. r=jandem
https://hg.mozilla.org/integration/autoland/rev/3d52ff211475
Part 2: Simplify return statement in IonBuilder::jsop_setarg. r=jandem
https://hg.mozilla.org/integration/autoland/rev/8a3508d85549
Part 3: Restructure IonBuilder::jsop_setarg. r=jandem
https://hg.mozilla.org/integration/autoland/rev/b89721d62b74
Part 4: Remove no longer used MSetFrameArgument node. r=jandem
https://hg.mozilla.org/integration/autoland/rev/84d19c5c6a7a
Part 5: Remove an outdated comment in IonBuilder::jsop_setarg. r=jandem
https://hg.mozilla.org/integration/autoland/rev/699e15941bae
Part 6: Remove AliasSet::FrameArgument. r=jandem
https://hg.mozilla.org/integration/autoland/rev/c367dad82129
Part 7: Update a method name in a comment. r=jandem

Comment 9

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/b0b1bbf5387c
https://hg.mozilla.org/mozilla-central/rev/3d52ff211475
https://hg.mozilla.org/mozilla-central/rev/8a3508d85549
https://hg.mozilla.org/mozilla-central/rev/b89721d62b74
https://hg.mozilla.org/mozilla-central/rev/84d19c5c6a7a
https://hg.mozilla.org/mozilla-central/rev/699e15941bae
https://hg.mozilla.org/mozilla-central/rev/c367dad82129