Last Comment Bug 713526 - IonMonkey: stub + PIC for SETPROP and SETNAME
: IonMonkey: stub + PIC for SETPROP and SETNAME
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Brian Hackett (:bhackett)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 715276 719433
Blocks: 684381 715511
  Show dependency treegraph
 
Reported: 2011-12-26 07:14 PST by Brian Hackett (:bhackett)
Modified: 2012-01-19 08:30 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (edde637d2661) (46.70 KB, patch)
2011-12-27 07:05 PST, Brian Hackett (:bhackett)
no flags Details | Diff | Splinter Review
patch (edde637d2661) (52.67 KB, patch)
2011-12-27 08:09 PST, Brian Hackett (:bhackett)
no flags Details | Diff | Splinter Review
patch (edde637d2661) (55.10 KB, patch)
2012-01-03 09:20 PST, Brian Hackett (:bhackett)
no flags Details | Diff | Splinter Review
patch (8e182985f782) (52.23 KB, patch)
2012-01-18 07:43 PST, Brian Hackett (:bhackett)
dvander: review+
Details | Diff | Splinter Review

Description Brian Hackett (:bhackett) 2011-12-26 07:14:48 PST
These opcodes are identical.
Comment 1 Brian Hackett (:bhackett) 2011-12-27 07:05:34 PST
Created attachment 584432 [details] [diff] [review]
WIP (edde637d2661)

Works on x86.
Comment 2 Brian Hackett (:bhackett) 2011-12-27 08:09:03 PST
Created attachment 584439 [details] [diff] [review]
patch (edde637d2661)
Comment 3 Brian Hackett (:bhackett) 2012-01-03 09:20:47 PST
Created attachment 585434 [details] [diff] [review]
patch (edde637d2661)

Clean up call instructions a bit.
Comment 4 David Anderson [:dvander] 2012-01-17 15:16:23 PST
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.
Comment 5 Brian Hackett (:bhackett) 2012-01-18 07:43:02 PST
Created attachment 589503 [details] [diff] [review]
patch (8e182985f782)
Comment 6 David Anderson [:dvander] 2012-01-18 19:11:13 PST
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
Comment 7 Brian Hackett (:bhackett) 2012-01-19 08:30:39 PST
https://hg.mozilla.org/projects/ionmonkey/rev/66106b3ac316

Note You need to log in before you can comment on or make changes to this bug.