681185 - IonMonkey: implement shifts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

implement JSOP_LSH,JSOP_RSH and JSOP_URSH

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: implement JSOP_LSH, JSOP_RSH and JSOP_URSH

implementation + tests

Comment 2

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

Comment on attachment 555039 [details] [diff] [review]
implement JSOP_LSH, JSOP_RSH and JSOP_URSH

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

::: js/src/ion/MIR.h
@@ +1148,5 @@
> +        return this;
> +    }
> +
> +    bool canBeNegative() {
> +        // solution is only negative when lhs < 0 and rhs & 0x1f == 0

Logic below looks good, but the name threw me off. How about "canOverflow"?

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +362,5 @@
> +            // But we need to bring the output back again from UINT32 to INT32.
> +            // Both representation overlap each other in the positive numbers. (in INT32)
> +            // So there is only a problem when solution (in INT32) is negative.
> +            if (ursh->canBeNegative()) {
> +                masm.cmpl(Imm32(0), ToRegister(lhs));

Nice, glad you caught this case and thanks for explaining it. Are the inputs to cmpl() swapped? It reads as bailoutIf(0 < reg)

::: js/src/ion/x86/Assembler-x86.h
@@ +288,5 @@
> +    }
> +    void sarl(const Register shift, const Register &dest) {
> +        JS_ASSERT(shift == ecx);
> +        masm.sarl_CLr(dest.code());
> +    }

How about instead of sarl(reg, reg) we call it sarl_cl(reg)? That way it's clear that it uses ecx but doesn't take in a bogo-parameter. Same with shrl/shll. Then you can share these functions in Assembler-x86-shared.

::: js/src/ion/x86/Lowering-x86.h
@@ +75,5 @@
>      bool assignSnapshot(LInstruction *ins);
>      void lowerUntypedPhiInput(MPhi *phi, uint32 inputPosition, LBlock *block, size_t lirIndex);
>      bool defineUntypedPhi(MPhi *phi, size_t lirIndex);
>  
> +    bool lowerForShifting(LInstructionHelper<1, 2, 0> *ins, MDefinition *mir, MDefinition *lhs, 

s/Shifting/Shift

Comment 3

[Hannes Verschore [:h4writer]]

http://hg.mozilla.org/projects/ionmonkey/rev/c1ec835d0f9d