Closed Bug 681185 Opened 10 years ago Closed 10 years ago

IonMonkey: implement shifts

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: h4writer, Assigned: h4writer)

Details

Attachments

(1 file)

implement JSOP_LSH,JSOP_RSH and JSOP_URSH
implementation + tests
Assignee: general → hv1989
Attachment #555039 - Flags: review?(dvander)
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
Attachment #555039 - Flags: review?(dvander) → review+
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.