Last Comment Bug 763884 - IonMonkey: (ARM) X%Y does not properly return -0.0
: IonMonkey: (ARM) X%Y does not properly return -0.0
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Linux
: -- normal (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-12 04:54 PDT by Marty Rosenberg [:mjrosenb]
Modified: 2012-07-27 17:50 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
/home/mrosenberg/patches/fixModulus-r0.patch (6.89 KB, patch)
2012-06-12 04:54 PDT, Marty Rosenberg [:mjrosenb]
Jacob.Bramley: review+
Details | Diff | Splinter Review

Description Marty Rosenberg [:mjrosenb] 2012-06-12 04:54:03 PDT
Created attachment 632207 [details] [diff] [review]
/home/mrosenberg/patches/fixModulus-r0.patch

A check is needed to see when a result of 0 is supposed to be -0.0
Comment 1 Jacob Bramley [:jbramley] 2012-06-13 02:00:35 PDT
Comment on attachment 632207 [details] [diff] [review]
/home/mrosenberg/patches/fixModulus-r0.patch

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

It looks good. Some comments and unconditional 'if' statements need cleaning up, and I think you need to add an assertion, but the logic looks correct.

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +540,5 @@
>      // Extract the registers from this instruction
>      Register lhs = ToRegister(ins->lhs());
>      Register rhs = ToRegister(ins->rhs());
> +    Register callTemp = ToRegister(ins->getTemp(2));
> +    if (true) { // mir->canBeNegativeZero()

What's that comment for? Should it be a TODO? If it's just leftover development code, remove the 'if' statement since it always executes.

@@ +571,5 @@
>      masm.passABIArg(lhs);
>      masm.passABIArg(rhs);
>      masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, __aeabi_idivmod));
> +    // If X%Y == 0 and X < 0, then we *actually* wanted to return -0.0
> +    if (true) { // mir->canBeNegativeZero()

Ditto.

::: js/src/ion/arm/LIR-arm.h
@@ +124,5 @@
>  // so we can modify them (and the function will).
>  // The other thre registers that can be trashed are marked as such. For the time
>  // being, the link register is not marked as trashed because we never allocate
>  // to the link register.
> +class LDivI : public LBinaryMath<3>

You didn't include any changes (other than whitespace) for ::visitDivI, only for ::visitModI. Did you intend to?

@@ +136,5 @@
>          setOperand(0, lhs);
>          setOperand(1, rhs);
>          setTemp(0, temp1);
>          setTemp(1, temp2);
> +        setTemp(2, callTemp);

Why not temp3? What's special about it? Is it callee-saved? (It needs to be.)

::: js/src/ion/arm/Lowering-arm.cpp
@@ +306,5 @@
>              return (assignSnapshot(lir) && define(lir, mod));
>          }
>      }
>      LModI *lir = new LModI(useFixed(mod->lhs(), r0), use(mod->rhs(), r1),
> +                           tempFixed(r2), tempFixed(r3), temp(LDefinition::GENERAL));

Does LDefinition::GENERAL make it callee-saved? I think you need to ASSERT in ::visitModI that the register is callee-saved, since things will break subtlely (and horribly) if a caller gets this wrong.
Comment 2 Marty Rosenberg [:mjrosenb] 2012-06-14 16:06:33 PDT
(In reply to Jacob Bramley [:jbramley] from comment #1)
> Comment on attachment 632207 [details] [diff] [review]
> /home/mrosenberg/patches/fixModulus-r0.patch
> 
> Review of attachment 632207 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It looks good. Some comments and unconditional 'if' statements need cleaning
> up, and I think you need to add an assertion, but the logic looks correct.
> 
> ::: js/src/ion/arm/CodeGenerator-arm.cpp
> @@ +540,5 @@
> >      // Extract the registers from this instruction
> >      Register lhs = ToRegister(ins->lhs());
> >      Register rhs = ToRegister(ins->rhs());
> > +    Register callTemp = ToRegister(ins->getTemp(2));
> > +    if (true) { // mir->canBeNegativeZero()
> 
> What's that comment for? Should it be a TODO? If it's just leftover
> development code, remove the 'if' statement since it always executes.
> 
yeah, I'll file a bug on that.  This is a method that exists for divi, but not modi, and it should probably exist for modi as well.
> @@ +571,5 @@
> >      masm.passABIArg(lhs);
> >      masm.passABIArg(rhs);
> >      masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, __aeabi_idivmod));
> > +    // If X%Y == 0 and X < 0, then we *actually* wanted to return -0.0
> > +    if (true) { // mir->canBeNegativeZero()
> 
> Ditto.
> 
> ::: js/src/ion/arm/LIR-arm.h
> @@ +124,5 @@
> >  // so we can modify them (and the function will).
> >  // The other thre registers that can be trashed are marked as such. For the time
> >  // being, the link register is not marked as trashed because we never allocate
> >  // to the link register.
> > +class LDivI : public LBinaryMath<3>
> 
> You didn't include any changes (other than whitespace) for ::visitDivI, only
> for ::visitModI. Did you intend to?
After inspecting it closer, it does not have the bug that modi has, and shouldn't have been modified.
> 
> @@ +136,5 @@
> >          setOperand(0, lhs);
> >          setOperand(1, rhs);
> >          setTemp(0, temp1);
> >          setTemp(1, temp2);
> > +        setTemp(2, callTemp);
> 
> Why not temp3? What's special about it? Is it callee-saved? (It needs to be.)
> 
I didn't give the other two meaningful names because they are simply there to make sure that none of the caller saved registers have any useful data in them.  Since I needed to reserve another register to hold a useful value, I decided to give it a someowhat meaningful name (I'm pretty bad at choosing good names)
> ::: js/src/ion/arm/Lowering-arm.cpp
> @@ +306,5 @@
> >              return (assignSnapshot(lir) && define(lir, mod));
> >          }
> >      }
> >      LModI *lir = new LModI(useFixed(mod->lhs(), r0), use(mod->rhs(), r1),
> > +                           tempFixed(r2), tempFixed(r3), temp(LDefinition::GENERAL));
> 
> Does LDefinition::GENERAL make it callee-saved? I think you need to ASSERT
> in ::visitModI that the register is callee-saved, since things will break
> subtlely (and horribly) if a caller gets this wrong.
I'll add the assertion, but there shouldn't be any way that we can assign a caller saved register to that register.
Comment 3 Marty Rosenberg [:mjrosenb] 2012-07-27 17:50:11 PDT
landed: http://hg.mozilla.org/projects/ionmonkey/rev/488cdd2e2267

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