Closed Bug 679493 Opened 13 years ago Closed 13 years ago

IonMonkey: implement JSOP_MUL

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: h4writer, Assigned: h4writer)

Details

Attachments

(5 files, 1 obsolete file)

implement the opcode JSOP_NEG in ionmonkey
Attached patch part 1: implement base + LNegI (obsolete) — Splinter Review
Implements base structure + LNegI. Next (and last part) is LNegD.

Few issues encountered when making this patch:
- LSRA fails when foldsto returns a MConstant:Double with an error. Greedy works so I think this is an issue with LSRA. Should I file a bug for an issue that only "not-checked-in" code can trigger? 
>> Assertion failure: a.isFloatReg(), at ../ion/shared/CodeGenerator-shared-inl.h:70
- Noticed JSOP_ADD has faults in his constant folding code. Will file bug for this.
Assignee: general → hv1989
Attachment #553581 - Flags: review?(dvander)
Comment on attachment 553581 [details] [diff] [review]
part 1: implement base + LNegI

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

After talk in irc with dvander it's better to have a different approach: Adjusting JSOP_NEG into MUL(x,-1).
Attachment #553581 - Flags: review?(dvander) → review-
note:
- issue 1: is normal. We can't change the type anymore during that phase.
- issue 2: solved by bug #678625
Summary: IonMonkey: implement JSOP_NEG → IonMonkey: implement JSOP_MUL
Using this bug for JSOP_MUL now.

TODO:
1) redirect JSOP_NEG to JSOP_MUL.
2) add MMul to LMulI to know if zero/overflow tests are needed.
3) make work with doubles
Attachment #553581 - Attachment is obsolete: true
implements 1) and 3).
This adds seperate overflow and zero checks. The checks are on by default and there are no function yet to disable a check. But it gives a foundation for later. When the idea of the fallible patch (dvander: see mail) get's approved, then this will change/simplify a little bit.
Attachment #554233 - Flags: review?(dvander)
Attachment #554262 - Flags: review?(dvander)
Attachment #554269 - Flags: review?(dvander)
Comment on attachment 554233 [details] [diff] [review]
Part 1 - basics, JSOP_MUL, MMul, LMulI and tests

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

Looks good, just some nits:

::: js/src/ion/shared/Assembler-x86-shared.h
@@ +482,5 @@
> +    }
> +    void mull(const Register &src, const Register &dest) {
> +        masm.imull_rr(src.code(), dest.code());
> +    }
> +    void mull(const Operand &src, const Register &dest) {

Could you rename these to imull? "mul" on x86 is an unsigned multiply.

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +219,5 @@
> +    if (rhs->isConstant()) {
> +        // Bailout on -0.0
> +        int32 constant = ToInt32(rhs);
> +        if (ins->snapshot() && constant <= 0) {
> +            Assembler::Condition bailoutCond = (constant == 0)?Assembler::LessThan : Assembler::Zero;

Give the ? some breathing room: ...) ? Assembler:: ... Also, though they're the same thing, Assembler::Equal is a little clearer when used with cmpl(). Nice work on simplifying this check, btw :)

@@ +225,5 @@
> +            if (bailoutIf(bailoutCond, ins->snapshot()))
> +                    return false;
> +        }
> +
> +        masm.mull(Imm32(ToInt32(rhs)), ToRegister(lhs));

You could peephole optimize here, if you wanted - neg for -1, xor for 0, add for 2.

@@ +238,5 @@
> +        if (ins->snapshot() && !bailoutIf(Assembler::Overflow, ins->snapshot()))
> +            return false;
> +
> +        // Bailout on 0 (could be -0.0)
> +        if (ins->snapshot()){

Space between ) and {

@@ +241,5 @@
> +        // Bailout on 0 (could be -0.0)
> +        if (ins->snapshot()){
> +            masm.cmpl(Imm32(0), ToRegister(lhs));
> +            if (!bailoutIf(Assembler::Zero, ins->snapshot()))
> +                return false;

This is what our past two JITs do - aggressively deoptimize on a zero result. Seems okay, I don't think it caused problems before. Fixing it would mean extra guards or more register pressure which would be sad.
Attachment #554233 - Flags: review?(dvander) → review+
Attachment #554262 - Flags: review?(dvander) → review+
Comment on attachment 554269 [details] [diff] [review]
Part 3: add seperate Overflow/Zero checks

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

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +219,5 @@
>  
>      if (rhs->isConstant()) {
>          // Bailout on -0.0
>          int32 constant = ToInt32(rhs);
> +        if (mul->needsZero() && constant <= 0) {

s/needsZero/canBeNegativeZero/

@@ +229,5 @@
>  
>          masm.mull(Imm32(ToInt32(rhs)), ToRegister(lhs));
>  
>          // Bailout on overflow
> +        if (mul->needsOverflow() && !bailoutIf(Assembler::Overflow, ins->snapshot()))

s/needsOverflow/canOverflow
Attachment #554269 - Flags: review?(dvander) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/b7c3e89e50cc
http://hg.mozilla.org/projects/ionmonkey/rev/0b8dece77de1
http://hg.mozilla.org/projects/ionmonkey/rev/71de4f9ef039

(In reply to David Anderson [:dvander] from comment #7)
> You could peephole optimize here, if you wanted - neg for -1, xor for 0, add
> for 2.
Sure, I'll do that. Do we want shifts for constants that are multiples of 2?

Testcases give the following error that will get fixes by patch in bug #678798:
Assertion failure: ins->valueNumber() != 0 at js/src/ion/ValueNumbering.cpp:273
I think shift doesn't set the overflow flag
The requested peephole optimizations.

shl indeed only sets overflow bit with 1-bit shift :(.
And cycle count of cmp + shl is almost the same as just running muli.
So no shift optimizations.
Attachment #554296 - Flags: review?(dvander)
Comment on attachment 554296 [details] [diff] [review]
part 4: peephole optimizations

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

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +241,5 @@
>                      return false;
>          }
>  
> +        // Should have been folded out.
> +        JS_ASSERT(constant != 1);

I wouldn't rely on this - we might want to turn off GVN for the baseline compiler.
Attachment #554296 - Flags: review?(dvander) → review+
Attached patch Part 4 improvedSplinter Review
This patch goes on top of part4!

(In reply to David Anderson [:dvander] from comment #12)
> ::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
> @@ +241,5 @@
> >                      return false;
> >          }
> >  
> > +        // Should have been folded out.
> > +        JS_ASSERT(constant != 1);
> 
> I wouldn't rely on this - we might want to turn off GVN for the baseline
> compiler.

1) I see. In that case I added the optimization to NOP.

2) I managed to add the shll optimization ;) (with performance increase, I suspect 3x). Only if constant is positive and canOverflow is false, though.
Attachment #554473 - Flags: review?(dvander)
Attachment #554473 - Flags: review?(dvander) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/bd11f4fae27a
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.