701964 - IonMonkey: Compile JSOP_LENGTH

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

Necessary for benchmarks.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: Implement JSOP_LENGTH.

Implement JSOP_LENGTH for Strings and Arrays.  Fallback to JSOP_GETPROP if the the type oracle does cannot guess the type.

Comment 2

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

Comment on attachment 581393 [details] [diff] [review]
Implement JSOP_LENGTH.

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

Looks good overall, but I have a few comments on code sharing and TI integration so I'd like to see the next iteration.

::: js/src/ion/IonBuilder.cpp
@@ +2871,5 @@
> +IonBuilder::jsop_length()
> +{
> +    TypeOracle::Unary signature = oracle->unaryOp(script, pc);
> +
> +    switch (signature.ival) {

Since the fast-paths in here are property accesses, there are special TI rules in play. For now it suffices to only build these fast-paths if the output type is known to be int32. (In TI, the output type set could be empty, which means it has not expected this code to run yet.)

@@ +2881,5 @@
> +        return true;
> +      }
> +
> +      case MIRType_Object:
> +        if (oracle->elementReadIsDense(script, pc)) {

Here, I think it's better to introduce a new Oracle API rather than extend "elementReadIsDense". You could include the popped-TypeSet in TypeOracle::Unary, and then check:

> if (signature.inTypes &&
>     !signature.inTypes->hasObjectFlags(cx, types::OBJECT_FLAG_NON_DENSE_ARRAY))
> {

@@ +2893,5 @@
> +            current->push(length);
> +
> +            return true;
> +        } else
> +            return jsop_getprop(info().getAtom(pc));

Nit: brace this, since the other half is braced.

::: js/src/ion/x86/CodeGenerator-x86.cpp
@@ +443,5 @@
> +    Register output = ToRegister(lir->output());
> +
> +    masm.loadPtr(lengthAndFlags, output);
> +    masm.shrl(Imm32(JSString::LENGTH_SHIFT), output);
> +    return true;

Let's hoist this outside of CG-$(ARCH) and into CodeGenerator.cpp - just add an "lshiftPtr(Imm32 amount, Register reg)" abstraction to all the MacroAssemblers.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: Implement JSOP_LENGTH.

Factor visitStringLength in ion/CG. (Add rshiftPtr in each MacroAssembler)
Add test case checking with type inference.
Add UnaryTypes and used it instead of Unary structure. (avoid calling twice poppedTypes)
Check for the expected return type.

Comment 4

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

Comment on attachment 581545 [details] [diff] [review]
Implement JSOP_LENGTH.

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

Great!

Comment 5

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/projects/ionmonkey/rev/9a3091bcc858