Closed
Bug 952992
Opened 10 years ago
Closed 10 years ago
Remove MPrepareCall and MPassArg instructions
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(1 file)
75.15 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter 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)
Comment 1•10 years ago
|
||
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....
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
(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.
Comment 4•10 years ago
|
||
Thanks for doing that! I think bug 915479 can indeed be considered as a duplicate.
Assignee | ||
Comment 5•10 years ago
|
||
Green on try: https://tbpl.mozilla.org/?tree=Try&rev=ea0b2f6b4a82
Assignee | ||
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
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+
Assignee | ||
Comment 8•10 years ago
|
||
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.
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1ed3e04b050b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in
before you can comment on or make changes to this bug.
Description
•