Remove MPrepareCall and MPassArg instructions

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Posted patch PatchSplinter Review
MPrepareCall and MPassArg have been a long-standing source of frustration and bugs, in particular PassArg. It has to be wrapped and unwrapped at the right time, it's not ok to capture a PassArg in a resume point and while I'm filing this bugzilla lists a PassArg bug as possible duplicate, bug 915479.

These instructions also make it hard to remove non-effectful MCall instructions, see bug 939581. Without these instructions, eliminating an MCall is as simple as eliminating any other instruction.

The attached patch removes these MIR-instructions. LStackArg instructions are inserted immediately before lowering the MCall; argument slots are no longer "nested". Performance on Kraken/SS/Octane seems to be either unaffected or a slight improvement. Fewer MIR instructions is also good for compilation time etc.

If this sticks, we can also remove JSOP_NOTEARG.

17 files changed, 160 insertions(+), 482 deletions(-)
Attachment #8351264 - Flags: review?(sstangl)
This is awesome!

With this, will it become feasible to do LICM and DCE on idempotent MCalls?  MPassArg/MPrepareCall were the reason I was told it wasn't feasible before....
I tried doing that, and at least at first glance it seems to work...  See the "Experiment in making DOM calls movable" attachment in bug 939581.
(In reply to Vacation Dec 19 to Jan 1 from comment #1)
> With this, will it become feasible to do LICM and DCE on idempotent MCalls? 
> MPassArg/MPrepareCall were the reason I was told it wasn't feasible
> before....

MCall is now just like any other instruction (it has a variable number of operands but that doesn't really matter), so that should work afaik. To get GVN you also have to override congruentTo, but it should be straight-forward: return false if the operand is not a call, else compare all MCall members and if they are equal forward to congruentIfOperandsEqual.
Thanks for doing that! I think bug 915479 can indeed be considered as a duplicate.
Comment on attachment 8351264 [details] [diff] [review]
Patch

Nicolas is around and offered to review this. It may be nice to get this in soonish.
Attachment #8351264 - Flags: review?(sstangl) → review?(nicolas.b.pierron)
Comment on attachment 8351264 [details] [diff] [review]
Patch

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

Nice.

Still we would need a follow-up bug/patch to address the abuse of the folded flag.

::: js/src/jit/IonBuilder.cpp
@@ +3815,5 @@
>  
>  bool
>  IonBuilder::jsop_notearg()
>  {
> +    //TODO: rm JSOP_NOTEARG

style-nit:
  // :TODO: Remove JSOP_NOTEARG. (Bug ABCDEF)

@@ +4928,5 @@
>      // to copy the arguments from the stack and call the function
>      if (inliningDepth_ == 0 && info().executionMode() != DefinitePropertiesAnalysis) {
>  
> +        MDefinition *vp = current->pop(); //XXX
> +        vp->setFoldedUnchecked();

nit: Replace //XXX by the following comment:

  The array argument corresponds to the argument object, as the JIT is implicitly reading the argument object in the next instruction, we need to prevent the deletion of the argument object from resume points, such as baseline will behave correctly after a bailout.

@@ +4961,5 @@
>      CallInfo callInfo(alloc(), false);
>  
>      // Vp
> +    MDefinition *vp = current->pop();
> +    vp->setFoldedUnchecked();

You can remove this setFoldedUnchecked, because:
 1/ Either it is marked in inlineScriptedCall.
 2/ Or used as argument of makeCall (thus used)

@@ +5203,5 @@
>              return nullptr;
>          }
>  
> +        callInfo.thisArg()->setFoldedUnchecked();
> +        callInfo.setThis(create);

We should not need setFoldedUnchecked here, as we are not adding any implicit use of thisArg.

::: js/src/jit/MCallOptimize.cpp
@@ +191,5 @@
>      const MathCache *cache = compartment->runtime()->maybeGetMathCache();
>      if (!cache)
>          return InliningStatus_NotInlined;
>  
> +    callInfo.setFoldedUnchecked();

Only flag |this|, as it is not used as argument.

I will accept these for now, as this only mirror the current behavior, but we should open a follow-up bug to reduce these such as we are not flagging all arguments, as we are adding uses for most of them.  We need to flag only arguments for which we are not adding any uses, such as inlined constants, or this-arguments.

::: js/src/jit/TypePolicy.cpp
@@ +655,5 @@
> +        MDefinition *arg = call->getArg(i);
> +        if (arg->type() != MIRType_Float32)
> +            continue;
> +
> +        MToDouble *replace = MToDouble::New(alloc, arg);

You should factor this out of the NoFloatPolicy, such as this code does not goes out of sync.
Attachment #8351264 - Flags: review?(nicolas.b.pierron) → review+
Blocks: 953284
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ed3e04b050b

Thanks, Nicolas.

(In reply to Nicolas B. Pierron [:nbp] from comment #7)
> style-nit:
>   // :TODO: Remove JSOP_NOTEARG. (Bug ABCDEF)

Done, filed bug 953284.

> nit: Replace //XXX by the following comment:

Done. Somehow I missed the XXX comment when I looked at the patch...

> >      // Vp
> > +    MDefinition *vp = current->pop();
> > +    vp->setFoldedUnchecked();
> 
> You can remove this setFoldedUnchecked, because:
>  1/ Either it is marked in inlineScriptedCall.
>  2/ Or used as argument of makeCall (thus used)

I got jit-test failures: we inline the call to f here: f.apply(x, arguments), and use the inlined arguments instead of the "arguments" parameter.

> We should not need setFoldedUnchecked here, as we are not adding any
> implicit use of thisArg.

This also causes assertion failures. Maybe we could whitelist this case in IonBuilder::traverseBytecode (see the #ifdef DEBUG checks there), but for now I kept the call as it shouldn't matter for performance (it's always an MConstant(UndefinedValue()) I think) and whitelisting could hide real bugs.

> Only flag |this|, as it is not used as argument.

Done.

> You should factor this out of the NoFloatPolicy, such as this code does not
> goes out of sync.

Good idea, done.
https://hg.mozilla.org/mozilla-central/rev/1ed3e04b050b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.