701093 - IonMonkey: Compile JSOP_GETELEM & JSOP_SETELEM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

We need JSOP_GETELEM and JSOP_SETELEM for fannkuch, bitops-nsieve-bits, crypto tests etc.

Supporting dense arrays, based on TI, should help a lot. For this, we need the following instructions:

- MGuardClass (we don't need this with TI)
- MDenseArrayLength (load initializedLength)
- MBoundsCheck (bailout if idx >= length)
- MLoadElement (load + bailout if hole and array is not known to be packed)
- MSlots (we already have this one from GETGNAME)

Good array performance is crucial, so any suggestions or ideas would be appreciated.

Comment 1

[Jan de Mooij [:jandem]]

GETELEM and SETELEM are mostly done, but working on some ARM patches first.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: WIP

Do we want to support inline paths for GETELEM and SETELEM without TI? It will make things more complex (we have to add MGuardClass, SETELEM needs a hole check because we cannot statically determine whether Array.prototype has indexed properties etc).

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch

Added x64 support and no longer aborts if TI is disabled. This required MGuardClass and a SETELEM hole check, but it makes testing a bit easier. Passes jit-tests on x86 and x64 with --ion and --ion-eager.

Brian, can you look at the TypeOracle changes, especially the elementWrite method?
Tomorrow I'll add some tests and ask somebody else to review the rest.

Comment 4

[Brian Hackett [Laid off!]]

Comment on attachment 575486 [details] [diff] [review]
Patch

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

::: js/src/ion/TypeOracle.cpp
@@ +204,5 @@
> +            types::TypeSet *elementTypes = object->getProperty(cx, JSID_VOID, false);
> +            if (!elementTypes)
> +                return MIRType_None;
> +
> +            MIRType type = getMIRType(elementTypes);

The type tag of elementTypes needs to be frozen via getKnownTypeTag, but I'm guessing that getMIRType already does this.

Comment 5

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

Comment on attachment 575486 [details] [diff] [review]
Patch

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

drive-by review:

::: js/src/ion/IonBuilder.cpp
@@ +2188,5 @@
> +{
> +    // Don't handle strings or other primitives.
> +    MIRType objType = current->peek(-2)->type();
> +    if (objType != MIRType_Object && objType != MIRType_Value)
> +        return abort("GETELEM not an object");

You cannot peek at the type of MIR objects during MIR construction, because Phis have not been computed yet. For example:

   var x = [];
   for (;;) {
      x[3];
      x = 90;
   }

@@ +2192,5 @@
> +        return abort("GETELEM not an object");
> +
> +    MIRType keyType = current->peek(-1)->type();
> +    if (keyType != MIRType_Int32 && keyType != MIRType_Double && keyType != MIRType_Value)
> +        return abort("GETELEM key not int or double");

Instead, you want to use the Type Oracle to tell you which case(s) to specialize for, and then make a TypePolicy which will automatically insert the MUnbox guards.

@@ +2258,5 @@
> +{
> +    // Don't handle strings or other primitives.
> +    MIRType objType = current->peek(-2)->type();
> +    if (objType != MIRType_Object && objType != MIRType_Value)
> +        return abort("SETELEM not an object");

Same comment as above.

::: js/src/ion/Lowering.cpp
@@ +614,5 @@
> +          LLoadElementV *lir = new LLoadElementV(useRegister(ins->slots()),
> +                                                 useRegisterOrConstant(ins->index()));
> +          if (ins->fallible() && !assignSnapshot(lir))
> +              return false;
> +          return defineBox(lir, ins);

nit: this case has an extra two-space indent.

::: js/src/ion/x64/CodeGenerator-x64.cpp
@@ +324,5 @@
> +
> +    if (load->index()->isConstant())
> +        loadUnboxedValue(Operand(slots, ToInt32(load->index()) * sizeof(js::Value)),
> +                         load->mir()->type(), load->output());
> +    else

Nit: Brace the if/else here since the bodies are multi-line

::: js/src/ion/x86/Assembler-x86.h
@@ +101,5 @@
>          REG_DISP,
>          FPREG,
>          SCALE,
> +        ADDRESS,
> +        INVALID

Good idea :)

Comment 6

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

(In reply to Jan de Mooij (:jandem) from comment #2)
> Created attachment 575194 [details] [diff] [review] [diff] [details] [review]
> WIP
> 
> Do we want to support inline paths for GETELEM and SETELEM without TI? It
> will make things more complex (we have to add MGuardClass, SETELEM needs a
> hole check because we cannot statically determine whether Array.prototype
> has indexed properties etc).

Nah, we don't have to worry about this. We should always make the Oracle API distinguish, somehow, whether guards are needed. This can be as simple as whether any specialization is returned at all. Without TI, or if TI tells us not to specialize, we can settle for ICs.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Patch

Add tests, address review comments and a bunch of other fixes and cleanups.

I decided to add the MToInt32 instruction in jsop_getelem_dense and jsop_setelem_dense. If we do this in the TypePolicy both MBoundsCheck and MLoadElement will add an MToInt32 instruction.

Not sure about calling masm.ToPayload from codegen, should we add a toPayload (lowercase t) public method?

@bhackett: yeah getMIRType calls getKnownTypeTag.

Comment 8

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

Comment on attachment 575871 [details] [diff] [review]
Patch

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

::: js/src/ion/MIR.h
@@ +1543,5 @@
>        : MUnaryInstruction(from)
>      {
>          setResultType(MIRType_Slots);
>          setIdempotent();
> +        JS_ASSERT(from->type() == MIRType_Object || from->type() == MIRType_Value);

I think this assert should just be removed (since types during MIR construction aren't realiable).

::: js/src/ion/shared/CodeGenerator-shared.cpp
@@ +198,5 @@
> +CodeGeneratorShared::createArraySlotOperand(Register slots, const LAllocation *index)
> +{
> +    if (index->isConstant()) {
> +        int32 offset = ToInt32(index);
> +        if (abs(offset) >= int32(INT32_MAX / sizeof(js::Value))) {

This check isn't needed - the overflow will be caught by the bounds check.

Comment 9

[Jan de Mooij [:jandem]]

Pushed: 

https://hg.mozilla.org/projects/ionmonkey/rev/5f40ec439376

Comment 10

[Nicolas B. Pierron [:nbp]]

There is a little issue with ARM builds: https://tbpl.mozilla.org/php/getParsedLog.php?id=7644152&tree=Ionmonkey

Marty, can we add the following modification to Assembler-arm.h ?

+    Operand (Register reg, int32 disp)
+      : Tag(DTR),
+        data(DTRAddr(reg, DtrOffImm(disp)))
+    { }

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Follow-up patch

Thanks, Nicolas. This patch moves createArraySlotOperand to CodeGenerator-x86-shared, it needs a different implementation on ARM. It renames loadInt32 to load32 to match MacroAssembler-arm. With these changes I can compile IM tip on ARM.

The patch also adds a write barrier for SETELEM, it's just like the one from MStoreSlot.

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 577900 [details] [diff] [review]
Follow-up patch

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

This sounds good to me.

::: js/src/ion/MIR.h
@@ +1782,5 @@
> +        return needsBarrier_;
> +    }
> +    void setNeedsBarrier(bool needsBarrier) {
> +        needsBarrier_ = needsBarrier;
> +    }

May be add a follow-up bug to use the MIR_FLAG_LIST for this boolean flag.

::: js/src/ion/TypeOracle.cpp
@@ +255,5 @@
> +bool
> +TypeInferenceOracle::propertyWriteNeedsBarrier(JSScript *script, jsbytecode *pc)
> +{
> +    types::TypeSet *types = script->analysis()->poppedTypes(pc, 2);
> +    return types->propertyNeedsBarrier(cx, JSID_VOID);

I think a comment would be great to explain the "2" such as "Check if SETELEM like instructions need a write barrier for the properties of their object argument (third one)."

Comment 13

[Jan de Mooij [:jandem]]

Pushed with the extra comment:

http://hg.mozilla.org/projects/ionmonkey/rev/9c0117082dee

Filed bug 706778 for the flag follow-up.