Last Comment Bug 701093 - IonMonkey: Compile JSOP_GETELEM & JSOP_SETELEM
: IonMonkey: Compile JSOP_GETELEM & JSOP_SETELEM
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: 684381
  Show dependency treegraph
 
Reported: 2011-11-09 10:51 PST by Jan de Mooij [:jandem]
Modified: 2011-12-01 02:14 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (40.38 KB, patch)
2011-11-17 08:06 PST, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Patch (61.46 KB, patch)
2011-11-18 09:17 PST, Jan de Mooij [:jandem]
bhackett1024: review+
Details | Diff | Splinter Review
Patch (60.11 KB, patch)
2011-11-21 08:24 PST, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Splinter Review
Follow-up patch (10.40 KB, patch)
2011-11-30 02:07 PST, Jan de Mooij [:jandem]
nicolas.b.pierron: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2011-11-09 10:51:34 PST
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] 2011-11-16 08:41:55 PST
GETELEM and SETELEM are mostly done, but working on some ARM patches first.
Comment 2 Jan de Mooij [:jandem] 2011-11-17 08:06:33 PST
Created attachment 575194 [details] [diff] [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).
Comment 3 Jan de Mooij [:jandem] 2011-11-18 09:17:02 PST
Created attachment 575486 [details] [diff] [review]
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 (:bhackett) 2011-11-20 07:52:07 PST
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] 2011-11-21 05:58:44 PST
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] 2011-11-21 06:02:00 PST
(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] 2011-11-21 08:24:28 PST
Created attachment 575871 [details] [diff] [review]
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] 2011-11-28 13:57:23 PST
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] 2011-11-29 10:13:49 PST
Pushed: 

https://hg.mozilla.org/projects/ionmonkey/rev/5f40ec439376
Comment 10 Nicolas B. Pierron [:nbp] 2011-11-29 16:16:24 PST
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] 2011-11-30 02:07:19 PST
Created attachment 577900 [details] [diff] [review]
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] 2011-11-30 11:22:56 PST
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] 2011-12-01 02:14:36 PST
Pushed with the extra comment:

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

Filed bug 706778 for the flag follow-up.

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