Closed Bug 707927 Opened 13 years ago Closed 12 years ago

IonMonkey: Compile JSOP_MOD

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: ejpbruel, Assigned: ejpbruel)

References

Details

Attachments

(2 files, 1 obsolete file)

      No description provided.
Depends on: 709423
Attaching what I have so far. Still totally untested, so no review request yet. No
support for ARM or doubles yet either.
Attachment #582981 - Attachment is patch: true
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.
Attachment #582981 - Attachment is obsolete: true
Attachment #583084 - Flags: review?
Attachment #583084 - Flags: review? → review?(sstangl)
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.
Attachment #583084 - Flags: review?(sstangl)
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.
Attachment #584445 - Flags: review?(sstangl)
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.
Attachment #584445 - Flags: review?(sstangl) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/ecde1352be48
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 714201
You need to log in before you can comment on or make changes to this bug.