Closed Bug 1116855 Opened 9 years ago Closed 9 years ago

Allow creating unboxed objects in interpreted constructors

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla38

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

Details

Attachments

(4 files, 3 obsolete files)

WIP
212.73 KB, patch
Details | Diff | Splinter Review
patch
104.87 KB, patch
jandem
: review+
Details | Diff | Splinter Review
JIT patch
162.63 KB, patch
jandem
: review+
Details | Diff | Splinter Review
fix AutoEnsureByteRegister
2.49 KB, patch
jandem
: review+
Details | Diff | Splinter Review 
Using 'new' to create objects is common and already has lots of analysis information associated with it which can be leveraged for other purposes.  This makes it a good place to start creating unboxed objects in the JS engine; the preliminary objects used by the new script analysis in particular is crucial for creating unboxed objects.
Attached patch WIP (obsolete) — — Splinter Review 
This is a basic WIP that adds a stage to the new script analysis where we look at the properties and property types on the preliminary objects and try to use an unboxed representation for the objects.  This patch is able to create unboxed objects in this way, but they are not yet optimized by Ion and can't be converted to normal native objects via new property sets, etc.  The rough plan is to add Ion optimizations to bring these up to speed, and see how benchmark / microbenchmark perf is affected before fleshing out the rest of the implementation.
Assignee: nobody → bhackett1024
Attached patch WIP (obsolete) — — Splinter Review 
New WIP with some Ion integration.  On 10.9 x86 this improves my octane-raytrace score from 93k to 109k (17%).  This is mainly due to the smaller size of the objects being allocated (raytrace does a lot of allocation) and improved code when we access double properties of those objects, since they are definitely-doubles rather than int32|double.
Attachment #8543069 - Attachment is obsolete: true
Attached patch WIP — — Splinter Review 
Yet another WIP with some performance improvements.  This improves the deltablue score so it is close to trunk, though there are still improvements that can be made and we should be able to get an improvement on this benchmark, since almost all non-arrays on deltablue are unboxed (the arrays presumably will be too once we have unboxed arrays).  I'm going to switch gears though and pare the optimizations out of this patch and finish implementing holes in the implementation so it can be landed pref'ed off.  It's pretty clear from octane-raytrace that we can get large performance improvements from unboxed objects and once optimizations are finished the only real disadvantage to these objects will be the cost of converting them to native objects if they are modified in a way unboxed objects don't support.  On octane this seems to only happen in box2d and I think the cost associated with this can be minimized.  e.g. don't fall back to polymorphic caches in Ion but introduce guard instructions to eagerly convert unboxed objects to natives when they flow into code that has seen converted natives before.  This is similar to what we do with convert-to-double-elements arrays (which we should be able to remove as part of the unboxed arrays work).
Attachment #8545390 - Attachment is obsolete: true
Attached patch patch — — Splinter Review 
Patch with JIT optimizations removed and a configuration option --unboxed-objects added for the shell.
Attachment #8547861 - Flags: review?(jdemooij)
Attached patch JIT patch (obsolete) — — Splinter Review 
This adds JIT ICs and inline Ion optimization paths for unboxed objects.
Attachment #8548328 - Flags: review?(jdemooij)
Depends on: 1121554
Sorry for the delay, will get to this now.
Comment on attachment 8547861 [details] [diff] [review]
patch

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

Nice, patch is simpler than I expected. r=me with comments below addressed.

::: js/src/jsobj.h
@@ +752,5 @@
>      static size_t offsetOfType() { return offsetof(JSObject, type_); }
>      js::HeapPtrTypeObject *addressOfType() { return &type_; }
>  
> +    // Maximum size in bytes of a JSObject.
> +    static const size_t MAX_BYTE_SIZE = 4 * sizeof(void *) + 16 * sizeof(JS::Value);

This can be sizeof(JSObject_Slots16).

::: js/src/vm/UnboxedObject.cpp
@@ +240,5 @@
> +                                      HandleId id, MutableHandleObject objp,
> +                                      MutableHandleShape propp)
> +{
> +    uint32_t index;
> +    if (JSID_IS_STRING(id) && !js_IdIsIndex(id, &index)) {

Is the js_IdIsIndex check necessary for correctness? Also below.

@@ +573,5 @@
> +    // Fill in all the unboxed object's property offsets, ordering fields from the
> +    // largest down to avoid alignment issues.
> +    uint32_t offset = 0;
> +
> +    size_t typeSizes[] = { 8, 4, 1 };

Nit: static const size_t

@@ +575,5 @@
> +    uint32_t offset = 0;
> +
> +    size_t typeSizes[] = { 8, 4, 1 };
> +
> +    Vector<int32_t, 0, SystemAllocPolicy> objectOffsets, stringOffsets;

Nit: s/0/8 or so to avoid some unnecessary mallocs in most cases.

@@ +590,5 @@
> +                    if (!stringOffsets.append(offset))
> +                        return false;
> +                }
> +                properties[j].offset = offset;
> +                offset += size;

Can you add a DebugOnly<size_t> that's incremented for each property we add here, and MOZ_ASSERT it equals templateShape->slotSpan() after the loop?

Just as an extra sanity-check to ensure all properties are handled.

@@ +605,5 @@
> +        return false;
> +
> +    // Construct the layout's trace list.
> +    if (!objectOffsets.empty() || !stringOffsets.empty()) {
> +        Vector<int32_t, 0, SystemAllocPolicy> entries;

Nit: s/0/8 or so here too.

@@ +614,5 @@
> +            !entries.append(-1))
> +        {
> +            return false;
> +        }
> +        int32_t *traceList = type->zone()->pod_malloc<int32_t>(entries.length() * sizeof(int32_t));

This should be just entries.length().

::: js/src/vm/UnboxedObject.h
@@ +39,5 @@
> +          : name(nullptr), offset(0), type(JSVAL_TYPE_MAGIC)
> +        {}
> +    };
> +
> +    typedef Vector<Property, 0, SystemAllocPolicy> PropertyVector;

Maybe worth giving this vector some inline storage, as there will always be > 0 properties?
Attachment #8547861 - Flags: review?(jdemooij) → review+
Comment on attachment 8547861 [details] [diff] [review]
patch

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

::: js/src/vm/UnboxedObject.cpp
@@ +478,5 @@
> +        UnboxedPlainObject::obj_setElement,
> +        UnboxedPlainObject::obj_getGenericAttributes,
> +        UnboxedPlainObject::obj_setGenericAttributes,
> +        UnboxedPlainObject::obj_deleteGeneric,
> +        nullptr, nullptr, /* watch/unwatch */

I think you either need to implement those or convert it to a native object in js::WatchProperty.
I'll review the second part today. One thing I forgot yesterday: we need a lot more tests for unboxed objects, for instance:

* Turning a property into getter/setter/non-writable.
* Adding a new property, either with defineProperty or simple obj.x = y.
* Using a proxy with an unboxed object.
* Assigning a double to an int32 property, undefined to null/object, etc.
* delete obj.prop.
* obj["prop"].
* ...
No longer blocks: 1122523
Comment on attachment 8548328 [details] [diff] [review]
JIT patch

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

::: js/src/jit/BaselineIC.cpp
@@ +8798,5 @@
> +
> +    if (fieldType_ == JSVAL_TYPE_OBJECT)
> +        EmitPreBarrier(masm, address, MIRType_Object);
> +    else if (fieldType_ == JSVAL_TYPE_STRING)
> +        EmitPreBarrier(masm, address, MIRType_String);

It'd be good to ensure we get assertion failures if a new GC thing is supported but not handled here. Maybe we can add a new function in UnboxedObject.h, like UnboxedTypeIsGCThing and MOZ_ASSERT it's false here?

Maybe we have that function already for JSValueType, I don't know.

::: js/src/jit/CodeGenerator.cpp
@@ +2344,5 @@
> +
> +            if (property->type == JSVAL_TYPE_OBJECT)
> +                masm.patchableCallPreBarrier(propertyAddr, MIRType_Object);
> +            else if (property->type == JSVAL_TYPE_STRING)
> +                masm.patchableCallPreBarrier(propertyAddr, MIRType_String);

Same assert comment here.

::: js/src/jit/IonBuilder.cpp
@@ +9792,5 @@
> +        break;
> +
> +      case JSVAL_TYPE_STRING:
> +        load = MLoadUnboxedString::New(alloc(), obj, scaledOffset,
> +                                       UnboxedPlainObject::offsetOfData());

Why do we have LoadUnboxedString etc and use LoadTypedArrayElement for the rest? It seems nicer to have a single MLoadUnboxedProperty or something with setResultType(unboxedType). That'd match LoadSlot/LoadElement and what you did with the Baseline GET/SET stubs.

Also LoadTypedArrayElement with result type MIRType_Boolean feels weird and I'm pretty sure it will confuse people. Maybe we could rename it to LoadTypedElement or LoadUnboxedElement.

@@ +9827,5 @@
> +        return true;
> +
> +    if (obj->type() != MIRType_Object) {
> +        MGuardObject *guard = MGuardObject::New(alloc(), obj);
> +        current->add(guard);

The type policy should handle this I think. Also below.

::: js/src/jit/IonCaches.cpp
@@ +2747,5 @@
> +    if (cx->zone()->needsIncrementalBarrier()) {
> +        if (unboxedType == JSVAL_TYPE_OBJECT)
> +            masm.callPreBarrier(address, MIRType_Object);
> +        else if (unboxedType == JSVAL_TYPE_STRING)
> +            masm.callPreBarrier(address, MIRType_String);

Same comment here.

::: js/src/jit/MacroAssembler.cpp
@@ +665,5 @@
> +        } else {
> +            // Reading null can't be possible here, as otherwise the result
> +            // would be a value (either because null has been read before or
> +            // because there is a barrier).
> +            loadPtr(address, output.typedReg().gpr());

We can assert that:

#ifdef DEBUG
Label ok;
masm.branchTestPtr(Assembler::NonZero, reg, reg, &ok);
masm.assumeUnreachable(...);
masm.bind(&ok);
#endif

@@ +732,5 @@
> +            Register valueScratch = value.reg().valueReg().scratchReg();
> +            MaybeSaveForExtract(this, valueScratch);
> +            Register reg = extractBoolean(value.reg().valueReg(), valueScratch);
> +            store8(reg, address);
> +            MaybeRestoreAfterExtract(this, valueScratch);

I think it'd be simpler and more efficient to add a storeUntypedBoolean() to the macro assemblers. Same for the other types.

On x86/ARM/MIPS we can just use value.payloadReg() directly and on x64 we can use the scratch register.

Also there's an annoying problem on x86 where |reg| has to be one of the byte op registers. (Or does this happen to be the case already?)

@@ +782,5 @@
> +                branchTestNumber(Assembler::NotEqual, value.reg().valueReg(), failure);
> +
> +            pushValue(value.reg().valueReg());
> +            loadUnboxedValue(Address(StackPointer, 0), MIRType_Double,
> +                             AnyRegister(ScratchDoubleReg));

We have unboxDouble(ValueOperand, FloatRegister) but that one doesn't handle int32. unboxValue(ValueOperand, AnyRegister) does though.

@@ +1281,5 @@
> +        const UnboxedLayout &layout = templateObj->as<UnboxedPlainObject>().layout();
> +
> +        // Initialize reference fields of the object, per UnboxedPlainObject::create.
> +        const int32_t *list = layout.traceList();
> +        if (list) {

Nit: if (const int32_t *list = layout.traceList()) {

::: js/src/jit/ScalarReplacement.cpp
@@ +113,5 @@
>          obj = objDefault;
>  
> +    // Don't optimize unboxed objects, which aren't handled by MObjectState.
> +    if (obj->is<UnboxedPlainObject>())
> +        return true;

Scalar replacement was a win on raytrace. This patch is also a win but maybe fixing this could make us even faster (or not, if these are different objects). Follow-up bug?

::: js/src/vm/UnboxedObject.h
@@ +96,5 @@
>      }
>  
> +    const Property *lookup(jsid id) const {
> +        uint32_t index;
> +        if (JSID_IS_ATOM(id) && !js_IdIsIndex(id, &index))

Nit: as discussed on IRC, it might be nice to avoid the IdIsIndex check by using atoms.
Attachment #8548328 - Flags: review?(jdemooij)
Comment on attachment 8547861 [details] [diff] [review]
patch

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

::: js/src/vm/UnboxedObject.cpp
@@ +184,5 @@
> +            return false;
> +    }
> +
> +    if (!SetClassAndProto(cx, obj, &PlainObject::class_, proto))
> +        return false;

Sorry for not realizing this sooner, but I was thinking about this last night and this is good fallback behavior. I think it's fair to assume this case will hit a lot, for instance if we have a Point with int32 x/y and we suddenly want to store a double.

SetClassAndProto is responsible for the performance problems we have with mutable __proto__ atm:

* TypeObjects get unknown properties, both the original and the new one.
* The whole protochain is reshaped, see bug 1108444 for how that affects code.

SetClassAndProto is a real performance hazard; we need it for __proto__ (we even emit a console warning there) and I don't like exposing it to normal, well-written JS.
Attachment #8547861 - Flags: review+
(In reply to Jan de Mooij [:jandem] from comment #11)
> and this is good fallback behavior.

Er, not good, I meant.
Comment on attachment 8547861 [details] [diff] [review]
patch

The SetClassAndProto call is something I intend to fix later on in this project.
Attachment #8547861 - Flags: review?(jdemooij)
(In reply to Brian Hackett (:bhackett) from comment #13)
> The SetClassAndProto call is something I intend to fix later on in this
> project.

Just curious, what's the plan? Can we make sure this is addressed before we enable unboxed objects?
(In reply to Jan de Mooij [:jandem] from comment #14)
> Just curious, what's the plan? Can we make sure this is addressed before we
> enable unboxed objects?

Instead of using SetClassAndProto, I want to add a specialized way of changing the type for the object that has good type information for the object's native type.  Because type sets containing the unboxed type will also implicitly contain the native type, we won't be able to optimize for the unboxed case anymore.  But at access sites that have only seen the unboxed and/or native types (and no other types), we should be able to optimize as we do currently (well, almost: the number of fixed slots will be smaller so we'll need to go to object slot arrays more often), except there will be a GVNable/hoistable instruction that eagerly converts the object to its native representation if it is unboxed.  Since unboxed->native is a one way transition, after it happens we don't need to worry about the object's structure becoming unstable across calls.

I'll definitely be fixing this before enabling unboxed objects, but first want to look more at benchmark perf.
Comment on attachment 8547861 [details] [diff] [review]
patch

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

::: js/src/vm/UnboxedObject.cpp
@@ +94,5 @@
> +    }
> +}
> +
> +Value
> +UnboxedPlainObject::getValue(uint32_t offset, JSValueType type)

Would be nicer to pass UnboxedLayout::Property here.
Comment on attachment 8547861 [details] [diff] [review]
patch

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

(In reply to Brian Hackett (:bhackett) from comment #15)
> I'll definitely be fixing this before enabling unboxed objects, but first
> want to look more at benchmark perf.

Sounds great, thanks.
Attachment #8547861 - Flags: review?(jdemooij) → review+
Great work here!  Could I check my understanding of the unboxed->native plan you described?

So we start with a type object TO1 that means "only unboxed" and create a bunch of instances.  Then a single instance gets an incompatible value so we create a new type object TO2 that means "native", we rebox that single instance to have native layout, we set some flag on TO1 saying "unboxed or native" and any code that was already compiled assuming TO1 meant "only unboxed" gets invalidated.  If new code is generated that can access TO1 objects, it will need to branch on whether the object is unboxed or native.  In the unboxed case, do we convert the object to be native?  Similarly, I assume after TO1 is marked as "unboxed or native" we stop creating unboxed TO1 instances and instead create only native TO2 instances?  If 'yes' to both, then it seems like gradually all TO1 instances would be converted over to TO2 instances (unless they sat dormant and were alive but never touched) and eventually TO1 could be finalized and swept from any TypeSets and thus code could eventually become fully specialized to TO2, which seems great.

Also, while I was initially worried about performance cliffs involving reboxing, it seems like the time spent reboxing is proportional to the time spent constructing unboxed objects and to the time spent accessing objects and thus reboxing should never change algorithmic complexity, just the constant.  The only exception I can think of is some unboxed object with tons of fields, but if you have some fixed upper bound on the number of fields you are willing to unbox, then that too becomes a constant.  Is that all right?

Lastly, will the new strategy avoid all the global prototype reshaping that Jan mentioned above?  That sounded scary.
(In reply to Luke Wagner [:luke] from comment #18)
> Great work here!  Could I check my understanding of the unboxed->native plan
> you described?
> 
> So we start with a type object TO1 that means "only unboxed" and create a
> bunch of instances.  Then a single instance gets an incompatible value so we
> create a new type object TO2 that means "native", we rebox that single
> instance to have native layout, we set some flag on TO1 saying "unboxed or
> native" and any code that was already compiled assuming TO1 meant "only
> unboxed" gets invalidated.  If new code is generated that can access TO1
> objects, it will need to branch on whether the object is unboxed or native. 
> In the unboxed case, do we convert the object to be native?

The plan I laid out in comment 15 would eagerly convert objects to natives.  The reason for this is that if there is code like 'x.f + x.g' and x is either unboxed or native, then if we eagerly convert the object to a native we only need one test on its type, before these operations execute.  If we don't eagerly convert the object, we would need to separately branch on the type at each access, and while we have MIR nodes to optimize for this case (e.g. MGetPropertyPolymorphic) that are better than using an IC, a definite property access is still faster.  The only real cost to eager unboxing vs. the present state of things (i.e. no unboxed objects at all) is that these objects will probably have a smaller size class and more of them will need dynamically allocated slots.

> Similarly, I
> assume after TO1 is marked as "unboxed or native" we stop creating unboxed
> TO1 instances and instead create only native TO2 instances?  If 'yes' to
> both, then it seems like gradually all TO1 instances would be converted over
> to TO2 instances (unless they sat dormant and were alive but never touched)
> and eventually TO1 could be finalized and swept from any TypeSets and thus
> code could eventually become fully specialized to TO2, which seems great.

During GC tracing we could convert objects to native if need be.  If we precompute the native shape the object will need and hold that strongly from the unboxed type, this process would not need to do any allocation other than the object slots, and if that OOM'ed the object could be left non-native.  Then, assuming there were no OOMs, after the first major GC we can fully specialize on the native type.

I hadn't thought about this; it does seem like a nice strategy, though it would be nice to do some testing and see how often native conversion happens in practice before deciding how much we want to optimize for this.

> Also, while I was initially worried about performance cliffs involving
> reboxing, it seems like the time spent reboxing is proportional to the time
> spent constructing unboxed objects and to the time spent accessing objects
> and thus reboxing should never change algorithmic complexity, just the
> constant.  The only exception I can think of is some unboxed object with
> tons of fields, but if you have some fixed upper bound on the number of
> fields you are willing to unbox, then that too becomes a constant.  Is that
> all right?

Unboxed objects don't have any dynamically allocated data, so the number of fields is limited by the maximum amount of data they can store, which is 136 bytes on x86.

Unboxed arrays will have this issue, however, since we could potentially need to box all the elements of an array of arbitrary length.  I imagine v8 has this issue with their unboxed arrays, however, though I don't know what they do to handle any associated issues.

> Lastly, will the new strategy avoid all the global prototype reshaping that
> Jan mentioned above?  That sounded scary.

Yes.  The prototype reshaping is needed because the prototype shape teleporting optimization wants a stable prototype chain.  Since converting an unboxed object to native does not change any prototype links or even the properties held by any object, no shape changes are necessary except on the transformed object.
Hmm, we were repeatedly reanalyzing 'new' scripts when the old information was detached.  I adjusted things so that we only detach the 'new' script info if the original analysis was successful, and also added an object flag to make sure we don't try to analyze the script again after its 'new' script info was detached.

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa85f08f9f76
(In reply to Jan de Mooij [:jandem] from comment #10)
> ::: js/src/jit/IonBuilder.cpp
> @@ +9792,5 @@
> > +        break;
> > +
> > +      case JSVAL_TYPE_STRING:
> > +        load = MLoadUnboxedString::New(alloc(), obj, scaledOffset,
> > +                                       UnboxedPlainObject::offsetOfData());
> 
> Why do we have LoadUnboxedString etc and use LoadTypedArrayElement for the
> rest? It seems nicer to have a single MLoadUnboxedProperty or something with
> setResultType(unboxedType). That'd match LoadSlot/LoadElement and what you
> did with the Baseline GET/SET stubs.
> 
> Also LoadTypedArrayElement with result type MIRType_Boolean feels weird and
> I'm pretty sure it will confuse people. Maybe we could rename it to
> LoadTypedElement or LoadUnboxedElement.

None of the ops used here are new; the same thing is done for typed objects, and the reuse of e.g. LoadTypedArrayElement dates to before I started working on the typed objects code.  I agree it's a mess, but cleaning it up shouldn't be done as part of this bug.

> @@ +9827,5 @@
> > +        return true;
> > +
> > +    if (obj->type() != MIRType_Object) {
> > +        MGuardObject *guard = MGuardObject::New(alloc(), obj);
> > +        current->add(guard);
> 
> The type policy should handle this I think. Also below.

Well, the constructor for the ops asserts this stuff and similar things are being done for existing paths like getPropTryDefiniteSlot.  So if this should be cleaned up it should be done as part of a larger effort.

> ::: js/src/jit/ScalarReplacement.cpp
> @@ +113,5 @@
> >          obj = objDefault;
> >  
> > +    // Don't optimize unboxed objects, which aren't handled by MObjectState.
> > +    if (obj->is<UnboxedPlainObject>())
> > +        return true;
> 
> Scalar replacement was a win on raytrace. This patch is also a win but maybe
> fixing this could make us even faster (or not, if these are different
> objects). Follow-up bug?

Yes, followup.
Attached patch JIT patch — — Splinter Review 
Updated JIT patch.  This also fixes some issues with native conversion that didn't show up in earlier testing.
Attachment #8548328 - Attachment is obsolete: true
Attachment #8556058 - Flags: review?(jdemooij)
Depends on: 1127133
Depends on: 1127167
Comment on attachment 8556058 [details] [diff] [review]
JIT patch

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

::: js/src/jit/IonBuilder.cpp
@@ +9764,5 @@
> +
> +    MInstruction *load;
> +    switch (unboxedType) {
> +      case JSVAL_TYPE_BOOLEAN:
> +        load = MLoadTypedArrayElement::New(alloc(), obj, scaledOffset, Scalar::Uint8,

Please file a follow-up bug to rename LoadTypedArrayElement?

::: js/src/jit/shared/MacroAssembler-x86-shared.h
@@ +653,5 @@
> +
> +    // Class which ensures that registers used in byte ops are compatible with
> +    // such instructions, even if the original register passed in wasn't. This
> +    // doesn't lead to great code but helps to simplify code generation.
> +    class AutoEnsureByteRegister {

Nit: I think it's worth mentioning this only matters on x86 (on x64 all registers are valid single byte regs).
Attachment #8556058 - Flags: review?(jdemooij) → review+
Depends on: 1128076
Non-JIT parts of the JIT patch.  I'm landing this in pieces because the whole patch is producing some pretty weird errors on try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e5c0911f112b
AutoEnsureByteRegister part of the patch.  This seems to have been what was causing the problems.  There were two bugs: first, the logic only worked on instructions that use the byte reg, not ones that write to it, and second, byte registers are necessary on byte-size store instructions but not loads (go figure).

https://hg.mozilla.org/integration/mozilla-inbound/rev/102f668f01d1
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/e5c0911f112b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
I just realized there's also a problem if you have:

  store8(edi, Address(eax, x));

We can't clobber eax here. Maybe we can use Operand::containsReg (or add that to Address/BaseIndex) and use either eax/ebx/ecx.
Flags: needinfo?(bhackett1024)
Oops, yes, good point.
Flags: needinfo?(bhackett1024)
Attachment #8557975 - Flags: review?(jdemooij)
Comment on attachment 8557975 [details] [diff] [review]
fix AutoEnsureByteRegister

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

Thanks.

::: js/src/jit/shared/MacroAssembler-x86-shared.h
@@ +662,1 @@
>          bool read_;

Nit: we can remove read_ I think as it's always true.
Attachment #8557975 - Flags: review?(jdemooij) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: