Closed
Bug 891910
Opened 8 years ago
Closed 8 years ago
IonMonkey: Remove bailing in LCallGeneric
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: h4writer, Assigned: h4writer)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
8.49 KB,
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
Currently we are bailing in LCallGeneric when object is not a JSFunction. But actually that can use the same vm path as not compiled functions.
Comment 1•8 years ago
|
||
If we do so, then we need to be careful and introduce a primitive which is compiled into a bailout MIR, such as we can keep the test suite working.
Assignee | ||
Comment 2•8 years ago
|
||
This patch fixes the issue. But I haven't tested it well enough to mark for review. Atm I'm using it as WIP patch. There are no issues I'm aware of, though
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #1) > If we do so, then we need to be careful and introduce a primitive which is > compiled into a bailout MIR, such as we can keep the test suite working. Can you elaborate more about this? Do currently depend on this bailing mechanism in LGenericCall ?
Flags: needinfo?(nicolas.b.pierron)
Comment 4•8 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #3) > (In reply to Nicolas B. Pierron [:nbp] from comment #1) > > If we do so, then we need to be careful and introduce a primitive which is > > compiled into a bailout MIR, such as we can keep the test suite working. > > Can you elaborate more about this? Do currently depend on this bailing > mechanism in LGenericCall ? The test suite use the fact that we do not yet handle calls to proxy to create a reproducible bailout: var bailout = Proxy.createFunction({}, Math.sin); function f2(a) { bailout(); return f2.arguments; };
Flags: needinfo?(nicolas.b.pierron)
Assignee | ||
Comment 5•8 years ago
|
||
I got one jit-test failure, due to error reporting difference. Should this get solved, or is this ok? jit-test/tests/jaeger/bug588338.js Without patch: TypeError: [1, 2, 3] is not a function With patch: TypeError: [1, 2, 3](...) is not a function
Assignee: general → hv1989
Attachment #775116 -
Attachment is obsolete: true
Attachment #783639 -
Flags: review?(sstangl)
Comment 6•8 years ago
|
||
Comment on attachment 783639 [details] [diff] [review] bug891910-bailing-lcallgeneric Review of attachment 783639 [details] [diff] [review]: ----------------------------------------------------------------- TypeError differences blow up the differential fuzzers. Would it be a pain to fix the discrepancy? ::: js/src/ion/CodeGenerator.cpp @@ +1636,5 @@ > Register objreg = ToRegister(call->getTempObject()); > Register nargsreg = ToRegister(call->getNargsReg()); > uint32_t unusedStack = StackOffsetOfPassedArg(call->argslot()); > ExecutionMode executionMode = gen->info().executionMode(); > + Label uncompiled, uncompiled2, thunk, makeCall, end; "uncompiled2" is unused. @@ +1654,3 @@ > masm.cmpPtr(nargsreg, ImmWord(&JSFunction::class_)); > + //if (!bailoutIf(Assembler::NotEqual, call->snapshot())) > + // return false; There's a bunch of leftover cruft here. @@ +1654,4 @@ > masm.cmpPtr(nargsreg, ImmWord(&JSFunction::class_)); > + //if (!bailoutIf(Assembler::NotEqual, call->snapshot())) > + // return false; > + masm.j(Assembler::NotEqual, &uncompiled); cmpPtr(); j(); is otherwise known as branchPtr(). There should also be a comment explaining that the "uncompiled" path is being used to force a bailout via CallVM. ::: js/src/ion/VMFunctions.cpp @@ +80,5 @@ > + } else { > + RootedValue rv(cx); > + if (!Invoke(cx, thisv, ObjectValue(*obj), argc, argvWithoutThis, &rv)) > + return false; > + *rval = rv; Although GetPCScript() and Monitor() cannot GC, it still seems strange to drop the Root on rv/rval early. ::: js/src/ion/VMFunctions.h @@ +567,5 @@ > cx_->runtime()->setIonReturnOverride(*rval_); > } > }; > > +bool InvokeFunction(JSContext *cx, HandleObject obj, uint32_t argc, Value *argv, Value *rval); The argument is called "obj0" in VMFunctions.cpp.
Attachment #783639 -
Flags: review?(sstangl)
Assignee | ||
Comment 7•8 years ago
|
||
This addresses comments. > TypeError differences blow up the differential fuzzers. Would it be a pain to fix the discrepancy? Apparently we don't need to worry about it. This still gives the same error when compiling --enable-more-deterministic (what fuzzers use). So I only had to patch that one script so it still checks the error, but not that tight. > There should also be a comment explaining that the "uncompiled" path is being used to force a bailout via CallVM. This doesn't force a bailout through CallVM. This just calls the function using callVM, so we don't bail! I renamed the "uncompiled" label to "invoke". Explains it better.
Attachment #783639 -
Attachment is obsolete: true
Attachment #786478 -
Flags: review?(sstangl)
Comment 9•8 years ago
|
||
Comment on attachment 786478 [details] [diff] [review] bug891910-bailing-lcallgeneric Review of attachment 786478 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/VMFunctions.cpp @@ +19,5 @@ > #include "ion/BaselineFrame-inl.h" > #include "vm/Interpreter-inl.h" > #include "vm/StringObject-inl.h" > > +//TODO: make style??? :-P
Attachment #786478 -
Flags: review?(sstangl) → review+
Assignee | ||
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9179e5001bb4
Flags: needinfo?(sstangl)
Comment 11•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9179e5001bb4
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in
before you can comment on or make changes to this bug.
Description
•