713526 - IonMonkey: stub + PIC for SETPROP and SETNAME

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

These opcodes are identical.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: WIP (edde637d2661)

Works on x86.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch (edde637d2661)

Comment 3

[Brian Hackett [Laid off!]]

Attachment: patch (edde637d2661)

Clean up call instructions a bit.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

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 [Laid off!]]

Attachment: patch (8e182985f782)

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

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 [Laid off!]]

https://hg.mozilla.org/projects/ionmonkey/rev/66106b3ac316