Closed Bug 998704 Opened 6 years ago Closed 5 years ago

Test coverage for integer div and mod

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: sunfish, Assigned: collares)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 976110's js/src/jit-test/tests/basic/testDivModWithIntMin.js adds some tests for div and mod by INT32_MIN and is a good start, but it's missing some interesting properties:

Truncation. Assertions like "assertEq(i % (-2147483648), i)" test the plain case, but the case where the operator is truncated, eg. with |0, is also interesting, since that will send it down different code paths in the JIT. I recommend leaving the existing asserts in place and adding additional asserts which test the truncated case.

JIT specialization. Even with --ion-eager, the first time the JIT sees some code, it won't have any dynamic type information. Consequently, it's a common idiom in JIT testcases to put the test code in a function and call the function twice (eg. see basic/bug757199.js). This way, in --ion-eager mode, the JIT will collect all the types in the first call, compile the function with type information, and then run the compiled code in the second call.
Depends on: 976110
Assignee: nobody → mau
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
Attachment #8435391 - Flags: review?(sunfish)
Comment on attachment 8435391 [details] [diff] [review]
patch

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

Overall looks good.

::: js/src/jit-test/tests/basic/testDivModWithIntMin.js
@@ +17,5 @@
> +testModNegativeZero();
> +testModNegativeZero();
> +
> +function testMinModulus1() {
> +    var intMin = -2147483648;

This var along the one above should be moved to the global scope, where it's less likely that the optimizer will be able to fold it away, skipping the codegen we're trying to test :-).

FWIW, I've filed bug 1021310 for adding a mechanism for doing this properly.

@@ +53,5 @@
> +    assertEq((-0) / -2147483648, 0);
> +
> +    assertEq(((-2147483648) / -2147483648)|0, 1);
> +    assertEq(((-2147483648) / -1)|0, -2147483648);
> +    assertEq(((-0) / -2147483648)|0, 0);

These are all also likely to be folded away, and should use an intMin variable as above.
Attachment #8435391 - Flags: review?(sunfish) → review+
Attached patch tests.patchSplinter Review
Attachment #8436023 - Flags: review+
Attachment #8436023 - Flags: checkin?
Attachment #8435391 - Attachment is obsolete: true
Comment on attachment 8436023 [details] [diff] [review]
tests.patch

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f059440e912
Attachment #8436023 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/4f059440e912
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.