Last Comment Bug 707927 - IonMonkey: Compile JSOP_MOD
: IonMonkey: Compile JSOP_MOD
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Eddy Bruel [:ejpbruel]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 709423
Blocks: 695770 714201
  Show dependency treegraph
 
Reported: 2011-12-06 07:25 PST by Eddy Bruel [:ejpbruel]
Modified: 2011-12-29 17:10 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
This should work (but probably doesn't) (16.25 KB, patch)
2011-12-19 15:26 PST, Eddy Bruel [:ejpbruel]
no flags Details | Diff | Splinter Review
This should work (and hopefully does) (16.17 KB, patch)
2011-12-19 23:10 PST, Eddy Bruel [:ejpbruel]
no flags Details | Diff | Splinter Review
Fixed folding rules (among other things) (18.14 KB, patch)
2011-12-27 08:58 PST, Eddy Bruel [:ejpbruel]
sstangl: review+
Details | Diff | Splinter Review

Description Eddy Bruel [:ejpbruel] 2011-12-06 07:25:50 PST

    
Comment 1 Eddy Bruel [:ejpbruel] 2011-12-19 15:26:45 PST
Created attachment 582981 [details] [diff] [review]
This should work (but probably doesn't)

Attaching what I have so far. Still totally untested, so no review request yet. No
support for ARM or doubles yet either.
Comment 2 Eddy Bruel [:ejpbruel] 2011-12-19 23:10:22 PST
Created attachment 583084 [details] [diff] [review]
This should work (and hopefully does)

Whilst running jit-tests.py I ran into an assertion in the register allocator. The cause was that I reused the same register (edx) for a temporary as for an output, thus confusing the allocator. This patch resolves that issue.

As it stands, this patch doesn't cause any more regressions on jit-tests.py, so it should be ready for review.
Comment 3 Sean Stangl [:sstangl] 2011-12-20 13:26:08 PST
Comment on attachment 583084 [details] [diff] [review]
This should work (and hopefully does)

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

This looks good. Only some small changes required.

::: js/src/ion/Lowering.cpp
@@ +443,5 @@
> +LIRGenerator::visitMod(MMod *ins)
> +{
> +    MDefinition *lhs = ins->lhs();
> +    MDefinition *rhs = ins->rhs();
> +    JS_ASSERT(lhs->type() == rhs->type());

The sides do not have to have the same type. Modding an integer by a string is valid (and results in a Number).

::: js/src/ion/MIR.cpp
@@ +586,5 @@
> +    if (MDefinition *folded = EvaluateConstantOperands(this))
> +        return folded;
> +
> +    // 0 % x -> 0
> +    // x % 0 -> NaN

The above folding rules are incorrect: for example, 0 % NaN -> NaN, not 0. We could handle all these rules by trying folds in a specific order:

1. x % NaN -> NaN
2. NaN % x -> NaN
3. x % 0 -> NaN
4. x % -0 -> NaN
5. 0 % x -> 0
- Handled above: 0 % 0 -> NaN
- Handled above: 0 % NaN -> NaN
- Handled above: 0 % -0 -> NaN
6. -0 % x -> -0
- Handled above: -0 % NaN -> NaN
- Handled above: -0 % 0 -> NaN
- Handled above: -0 % -0 -> NaN
7. Infinity % x -> NaN
8. -Infinity % x -> NaN
9. x % Infinity -> x
- Handled above: Infinity % Infinity -> NaN
- Handled above: NaN % Infinity -> NaN
- Handled above: -Infinity % Infinity -> NaN
10. x % -Infinity -> x
- Handled above: Infinity % -Infinity -> NaN
- Handled above: NaN % -Infinity -> NaN
- Handled above: -Infinity % -Infinity -> NaN

I think that is correct and sufficient.

::: js/src/ion/shared/CodeGenerator-x86-shared.cpp
@@ +579,5 @@
> +{
> +    const LDefinition *remainder = ins->remainder();
> +    const LAllocation *lhs = ins->lhs();
> +    const LAllocation *rhs = ins->rhs();
> +

It is idiomatic to write const Register lhsReg = ToRegister(lhs);, e.g., so that we don't have to clutter below with ToRegister() wrappers.

@@ +584,5 @@
> +    JS_ASSERT(ToRegister(remainder) == edx);
> +    JS_ASSERT(ToRegister(lhs) == eax);
> +
> +    // We don't have to prevent division by zero, since lhs % 0 is reduced
> +    // to inf during constant folding (see MMod::foldsTo)

We still do -- the rhs may have 0 passed in via a non-constant variable, which would not be folded.

@@ +591,5 @@
> +    masm.xorl(edx, edx);
> +    masm.testl(ToRegister(lhs), ToRegister(lhs));
> +    Label negative;
> +    masm.j(Assembler::Signed, &negative);
> +    masm.idiv(ToRegister(rhs));

idiv must be preceded by masm.cdq() to sign-extend eax into edx. If we do it before the j(), then we only need to output the instruction once. Requires a comment (can take from visitDivI).

@@ +597,5 @@
> +    masm.jmp(&join);
> +
> +    // else remainder = -(-lhs % rhs)
> +    masm.bind(&negative);
> +    masm.negl(ToOperand(lhs));

We should write and use a "const Register &reg" version of negl(). Wrapping with an Operand should not be necessary.

::: js/src/ion/x64/LOpcodes-x64.h
@@ +46,5 @@
>      _(Box)                          \
>      _(Unbox)                        \
>      _(UnboxDouble)                  \
> +    _(DivI)                         \
> +    _(ModI)

This should also go in LOpcodes-x86.h and LOpcodes-arm.h.

Really, all these instructions should be defined in ion/LOpcodes.h anyway, since they are not actually platform-dependent.

Could we move ModI there for this patch, and then address the others in a later patch?

::: js/src/ion/x64/Lowering-x64.cpp
@@ +249,5 @@
>  }
>  
>  bool
> +LIRGeneratorX64::lowerModI(MMod *mod)
> +{

Does only assignSnapshot() prevent us from moving this function to Lowering-x86-shared.cpp?

::: js/src/jsinterp.cpp
@@ +2826,5 @@
>          double d1, d2;
>          if (!ToNumber(cx, regs.sp[-2], &d1) || !ToNumber(cx, regs.sp[-1], &d2))
>              goto error;
>          regs.sp--;
> +        regs.sp[-1].setNumber(NumberMod(d1, d2));

This actually changes behavior very slightly -- if the double result can be coerced to an Int32, then it now is, where previously the result would have remained as a double.

I think this is fine, though.
Comment 4 Eddy Bruel [:ejpbruel] 2011-12-27 08:58:01 PST
Created attachment 584445 [details] [diff] [review]
Fixed folding rules (among other things)

I've removed the invalid assertion in LIRGenerator::visitMod and added the folding rules you suggested (I am not too sure about the way I obtain the values for NaN and Infinity, so you might want to double check that). I've also removed the ToRegister wrapper clutter, put in a comment concerning the use of masm.cdq, and added a const Register &reg version of negl().

I am not too sure whether assignSnapshot is the only thing preventing us from moving LIRGeneratorX64::lowerModI to Lowering-x86-shared.cpp. I've put it in Lowering-x64.cpp and Lowering-x86.cpp because LIRGeneratorX64::lowerDivI is also defined there. In any case, the 32-bit version uses different registers than the 64-bit one, but it is my understanding that these registers map to the same enum values. It might be possible to share this code, but for the time being I've left it where it is.

You mentioned the slight change in behavior my patch introduces in js/src/jsinterp.cpp shouldn't be a problem, so I've left that part of the patch as it is.
Comment 5 Sean Stangl [:sstangl] 2011-12-27 12:00:22 PST
Comment on attachment 584445 [details] [diff] [review]
Fixed folding rules (among other things)

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

r+ with minor folding changes. Can you make the changes and push? If not, uploading another version is fine.

::: js/src/ion/MIR.cpp
@@ +587,5 @@
> +        return folded;
> +
> +    JSRuntime *rt = GetIonContext()->cx->runtime;
> +    double NaN = JSVAL_TO_DOUBLE(rt->NaNValue);
> +    double Inf = JSVAL_TO_DOUBLE(rt->positiveInfinityValue);

I think just .toDouble() should be sufficient.

@@ +590,5 @@
> +    double NaN = JSVAL_TO_DOUBLE(rt->NaNValue);
> +    double Inf = JSVAL_TO_DOUBLE(rt->positiveInfinityValue);
> +
> +    // NaN % x -> NaN
> +    if (IsConstant(lhs(), NaN))

IsConstant() calls def->toConstant->value().toNumber(), which will trip an assertion if lhs is, for example, a constant string that can be converted to a Number (via ToNumber()) but is not itself a Number.

Folding should use the 3-argument version of ToNumber() on both the LHS and RHS, returning |this| if either cannot be converted. Then if both succeed, we have the double versions.

Macros JSDOUBLE_IS_NAN() and JSDOUBLE_IS_NEGZERO() exist; we can use those here. For infinities, there is JSDOUBLE_IS_INFINITE() and JSDOUBLE_IS_NEG().

Comparing against named doubles (such as NaN and Inf above) looks nicer than using macros, but the macros could be used to condense logic at your discretion.
Comment 6 Sean Stangl [:sstangl] 2011-12-29 17:06:37 PST
http://hg.mozilla.org/projects/ionmonkey/rev/ecde1352be48

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