Closed
Bug 713526
Opened 13 years ago
Closed 12 years ago
IonMonkey: stub + PIC for SETPROP and SETNAME
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bhackett1024, Assigned: bhackett1024)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
52.23 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
These opcodes are identical.
Assignee | ||
Comment 1•13 years ago
|
||
Works on x86.
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #584432 -
Attachment is obsolete: true
Attachment #584439 -
Flags: review?(dvander)
Assignee | ||
Comment 3•13 years ago
|
||
Clean up call instructions a bit.
Attachment #584439 -
Attachment is obsolete: true
Attachment #584439 -
Flags: review?(dvander)
Attachment #585434 -
Flags: review?(dvander)
Comment on attachment 585434 [details] [diff] [review] patch (edde637d2661) Review of attachment 585434 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the late review. Looks fine for the most part but it looks like a decent amount of rebasing is needed so I'd like a quick pass over the final version. I promise I'll get to the next one within 24 hours. ::: js/src/ion/CodeGenerator.cpp @@ +691,5 @@ > return true; > } > > +const MGenericSetPropertyOrName * > +CodeGenerator::getCallSetPropertyOrNameData(LInstruction *ins, ConstantOrRegister *pvalue) This function is kind of confusing and doesn't seem to fit in. Maybe it's just the name. If all of these L*SetProperty* functions share from one base class it could just be: ToSetPropertyRHS() or even ToConstantOrRegister() and look similar to ToValue(), ToInt32(), ToRegister(), etc. @@ +736,5 @@ > + ? FunctionInfo<pf>(SetProperty<true>) > + : FunctionInfo<pf>(SetProperty<false>); > + const Register objReg = ToRegister(ins->getOperand(0)); > + > + pushValueArg(value); This can just be pushArg() now. ::: js/src/ion/IonCaches.cpp @@ +235,5 @@ > + > + return obj->setGeneric(cx, ATOM_TO_JSID(atom), &value, cache.strict()); > +} > + > +const VMFunction ion::SetPropertyOrNameCacheFun( This should be declared like GetPropertyOrNameFn - near the use site, using FunctionInfo<>() ::: js/src/ion/IonCaches.h @@ +95,5 @@ > +}; > + > +struct ConstantOrRegisterSpace > +{ > + char data_[sizeof(ConstantOrRegister)]; I'm not sure if GCC will like this aliasing. If not use AlignedStorage<> ::: js/src/ion/IonMacroAssembler.h @@ +204,5 @@ > + subPtr(Imm32(sizeof(Value)), StackPointer); > + framePushed_ += sizeof(Value); > + } > + > + void PopValueSpace() { These two functions aren't needed anymore. ::: js/src/ion/IonRegisters.h @@ +306,5 @@ > + // Whether a constant value is being stored. > + bool constant_; > + > + // Space to hold either a Value or a TypedOrValueRegister. > + char data[tl::Max<sizeof(Value), sizeof(TypedOrValueRegister)>::result]; This will make gcc angry union U { AlignedStorage2<TypedOrValueRegister> typed; AlignedStorage2<Value> value; } data; might work better ::: js/src/ion/LIR-Common.h @@ +1117,5 @@ > } > }; > > +// Call a stub to perform a property or name assignment of a generic value. > +class LCallSetPropertyOrNameV : public LCallNoResultInstructionHelper<1 + BOX_PIECES, 0> I'd just take the "OrName" off all these LIR nodes and MIR as well. Unlike getname this decomposition is explicit in the bytecode. @@ +1177,5 @@ > +}; > + > +// Patchable jump to stubs generated for a SetProperty cache, which stores a > +// value of a known type. > +class LCacheSetPropertyOrNameT : public LInstructionHelper<0, 2, 0> These should get names consistent with the GetProperty nodes (same for MIR). That, or the existing ones should be changed to conform to this style. I think I prefer Cache at the end, FWIW. ::: js/src/ion/LIR.h @@ +809,5 @@ > + public: > + virtual bool isCall() const { > + return true; > + } > + virtual RegisterSet &spillRegs() const { This class shouldn't be needed anymore. ::: js/src/ion/TypeOracle.h @@ +251,5 @@ > } > > +static inline JSValueTag > +MIRTypeToTag(MIRType type) > +{ This removes the assert that we should never [Un]Box(Undefined|Null) which has caught bugs in the past. It'd be nice to get that back. ::: js/src/ion/arm/MacroAssembler-arm.h @@ +356,5 @@ > + void storeValue(JSValueType type, Register reg, Address dest) { > + JS_NOT_REACHED("NYI"); > + } > + void storeValue(const Value &val, Address dest) { > + JS_NOT_REACHED("NYI"); These should be implemented to land - r? :Marty @@ +582,5 @@ > void Push(Register reg) { > JS_NOT_REACHED("feature NYI"); > } > + void Push(ImmGCPtr ptr) { > + JS_NOT_REACHED("feature NYI"); ditto ::: js/src/ion/shared/CodeGenerator-shared.h @@ +232,5 @@ > + if (v.constant()) > + pushValueArg(v.value()); > + else > + pushValueArg(v.reg()); > + } bug 715276 should have subsumed needing these functions - if the specializations are still needed, they should go in the MacroAssembler.
Attachment #585434 -
Flags: review?(dvander)
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #585434 -
Attachment is obsolete: true
Attachment #589503 -
Flags: review?(dvander)
Comment on attachment 589503 [details] [diff] [review] patch (8e182985f782) Review of attachment 589503 [details] [diff] [review]: ----------------------------------------------------------------- Nice, thanks for taking this. ::: js/src/ion/CodeGenerator.cpp @@ +1020,5 @@ > return true; > } > > +const MGenericSetProperty * > +CodeGenerator::getCallSetPropertyData(LInstruction *ins, ConstantOrRegister *pvalue) This function still bothers me for some reason. It's either the outparam or the name or both. It looks like it would become a lot simpler to just return a ConstantOrRegister and then have the caller get the MIR itself. ::: js/src/ion/Lowering.cpp @@ +952,5 @@ > +{ > + LUse obj = useRegister(ins->obj()); > + > + LInstruction *lir; > + if (ins->value()->type() == MIRType_Value) { This function should not generate the cache versions for ARM, if they don't work yet - please file a follow-up and cite it here. ::: js/src/ion/x64/CodeGenerator-x64.cpp @@ +104,5 @@ > > static inline JSValueShiftedTag > MIRTypeToShiftedTag(MIRType type) > { > + JS_ASSERT(type != MIRType_Double); && type >= MIRType_Boolean
Attachment #589503 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 7•12 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/66106b3ac316
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•