Last Comment Bug 701964 - IonMonkey: Compile JSOP_LENGTH
: IonMonkey: Compile JSOP_LENGTH
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
Mentors:
Depends on:
Blocks: 684381
  Show dependency treegraph
 
Reported: 2011-11-11 18:00 PST by Nicolas B. Pierron [:nbp]
Modified: 2011-12-21 19:10 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement JSOP_LENGTH. (22.76 KB, patch)
2011-12-13 13:27 PST, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Implement JSOP_LENGTH. (25.53 KB, patch)
2011-12-13 22:37 PST, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2011-11-11 18:00:00 PST
Necessary for benchmarks.
Comment 1 Nicolas B. Pierron [:nbp] 2011-12-13 13:27:05 PST
Created attachment 581393 [details] [diff] [review]
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] 2011-12-13 15:06:07 PST
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] 2011-12-13 22:37:27 PST
Created attachment 581545 [details] [diff] [review]
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] 2011-12-16 11:54:04 PST
Comment on attachment 581545 [details] [diff] [review]
Implement JSOP_LENGTH.

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

Great!
Comment 5 Nicolas B. Pierron [:nbp] 2011-12-21 19:10:20 PST
https://hg.mozilla.org/projects/ionmonkey/rev/9a3091bcc858

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