Closed
Bug 684402
Opened 13 years ago
Closed 13 years ago
IonMonkey: Compile JSOP_GETGNAME
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: dvander, Assigned: dvander)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
80.59 KB,
patch
|
bhackett1024
:
review+
sstangl
:
review+
|
Details | Diff | Splinter Review |
The easiest of the property access opcodes - this should start showing us how to integrate Type Inference information for heap accesses as well.
Assignee | ||
Comment 1•13 years ago
|
||
Provides MSlots, MLoadSlot, LSlots, LLoadSlotV. With this we can run scripts with GETGLOBAL. Left todo: * Integrate with TI and add typed LIR. * x64 support. * Tests.
Assignee | ||
Updated•13 years ago
|
Summary: IonMonkey: Compile JSOP_GETGLOBAL → IonMonkey: Compile JSOP_GETGNAME
Assignee | ||
Comment 3•13 years ago
|
||
Adds TypeBarrier, GuardShape, LoadSlots, and LoadSlotV instructions. Remaining work to do: * x64, ARM support * typed loads * tests
Attachment #558711 -
Attachment is obsolete: true
Assignee | ||
Comment 4•13 years ago
|
||
Adds x64 support, typed loads. ARM and actual barrier functionality soon.
Attachment #565854 -
Attachment is obsolete: true
Assignee | ||
Comment 5•13 years ago
|
||
Brian, would you mind looking at the TI integration in IonBuilder.cpp and TypeOracle.cpp?
Attachment #565872 -
Attachment is obsolete: true
Attachment #565983 -
Flags: review?(bhackett1024)
Assignee | ||
Updated•13 years ago
|
Attachment #565983 -
Flags: review?(sstangl)
Updated•13 years ago
|
Attachment #565983 -
Flags: review?(bhackett1024) → review+
Comment 6•13 years ago
|
||
Comment on attachment 565983 [details] [diff] [review] part 1: everything but ARM Review of attachment 565983 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/Bailouts.cpp @@ +246,5 @@ > JS_NOT_REACHED("NYI"); > } > > + // TypeBarriers on non-idempotent ops not supported yet. > + JS_ASSERT(iter.bailoutKind() == Bailout_Normal); For the sake of fuzzing, it's useful for NYI assertions to contain the string "NYI": if (iter.bailoutKind() != Bailout_Normal) JS_NOT_REACHED("NYI: TypeBarriers on non-idempotent ops."); ::: js/src/ion/IonBuilder.cpp @@ +1907,5 @@ > + // prototype chain, then if obj has a defined property now, and the > + // property has a default or method shape, the only way it can produce > + // undefined in the future is if it is deleted. Deletion causes type > + // properties to be explicitly marked with undefined. > + *result = false; Suggest "isKnownConstant" instead of "result". @@ +1949,5 @@ > + // instruction is not idempotent, even if it has a singleton type, > + // there must be a resume point capturing the original def. > + current->push(ins); > + if (!ins->isIdempotent() && !resumeAfter(ins)) > + return false; jsop_call() also creates a resume point after the operation. Presumably it wants to do so whether or not a type barrier needs to follow. That is fine, but the logic is sort of hidden by the naming. This function's intent is to push a type barrier if required; otherwise, to push |ins|. "pushInstructionWithTypeBarrier()"? @@ +1952,5 @@ > + if (!ins->isIdempotent() && !resumeAfter(ins)) > + return false; > + > + if (!observed || observed->unknown()) > + return true; Renaming to something like the above would also make more obvious errors where |ins| is duplicated on the stack by the caller of this function. ::: js/src/ion/IonMacroAssembler.h @@ +145,5 @@ > > // Emits a test of a value against all types in a TypeSet. A scratch > // register is required. > + template <typename T> > + void testTypeSet(const T &address, types::TypeSet *types, Register scratch0, nit: s/0// ::: js/src/ion/LIR-Common.h @@ +715,5 @@ > + } > +}; > + > +// Load a value from an object's dslots or a slots vector. > +class LLoadSlotT : public LInstructionHelper<1, 1, 0> Comment should distinguish from LLoadSlotV by mentioning the typed load. ::: js/src/ion/MIR.h @@ +1011,5 @@ > + public: > + enum Mode { > + Fallible, > + Infallible, > + TypeBarrier Comment on effect of each mode? @@ +1040,5 @@ > + MDefinition *input() const { > + return getOperand(0); > + } > + BailoutKind bailoutKind() const { > + JS_ASSERT(fallible()); This assertion looks strange, but its utility makes sense. Maybe a comment would help. ::: js/src/ion/Snapshots.cpp @@ +51,5 @@ > // Snapshot structure: > // > // [ptr] Debug only: JSScript * > // [vwu] pc offset > +// [vwu] bits (n-31]: # of slots, including nargs (max 65535). Well... max 2^(32-n). We should have this value available as a constant, so we can abort on overfullness during SSA construction. ::: js/src/ion/TypeOracle.h @@ +85,5 @@ > virtual types::TypeSet *parameterTypeSet(JSScript *script, size_t index) { return NULL; } > + virtual types::TypeSet *propertyRead(JSScript *script, jsbytecode *pc, > + types::TypeSet **barrier) { > + *barrier = NULL; > + return NULL; Can be "return (*barrier = NULL);", if that helps condense to the above line.
Attachment #565983 -
Flags: review?(sstangl) → review+
Assignee | ||
Comment 7•13 years ago
|
||
> For the sake of fuzzing, it's useful for NYI assertions to contain the > string "NYI": > if (iter.bailoutKind() != Bailout_Normal) > JS_NOT_REACHED("NYI: TypeBarriers on non-idempotent ops."); Okay, I'll keep that in mind for the future - this code goes away in a follow-up patch. > > Suggest "isKnownConstant" instead of "result". Good idea. > This function's intent is to push a type barrier if required; otherwise, to > push |ins|. > > "pushInstructionWithTypeBarrier()"? This stuff is all wrong anyway, it's fixed in the "type barriers for calls" patch. You're right though, the resume point should be created outside of the type barrier logic. > Well... max 2^(32-n). We should have this value available as a constant, so > we can abort on overfullness during SSA construction. nargs is limited to 65535 by the emitter (same for nslots and therefore nfixed). Pushed with nits fixed: http://hg.mozilla.org/projects/ionmonkey/rev/e14a523e99d3
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 8•13 years ago
|
||
Comment on attachment 565983 [details] [diff] [review] part 1: everything but ARM Review of attachment 565983 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonLIR.h @@ +988,5 @@ > +# define BOX_OUTPUT_ACCESSORS() \ > + const LDefinition *outputValue() { \ > + return getDef(0); \ > + } > +#endif I think we may want some kind of LValueDefinition to factor these "if defined(JS_NUNBOX32)" & "elif defined(JS_PUNBOX64)". Your current implementation implies that any code that has to use a Value must either reproduce this "#ifdef" or do 2 implementation of the same function based on the architecture. These separation caused by Value representation appear in the Lowering-shared-inl.h and in specialized Lowering implementation.
You need to log in
before you can comment on or make changes to this bug.
Description
•