Add more tests to mul-exhaustive for constant left/right parameters.

Add more tests to mul-exhaustive for constant left/right operands.

Make MJSUnit able to distinguish 0 and -0.

Committed: http://code.google.com/p/v8/source/detail?r=7368

[Kevin Millikin (Chromium), 2011-03-21 11:25:39]

mul-exhaustive.js should be simplified.  It seems too complicated and it's not
obvious why it has to be.

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/math-sqrt.js
File test/mjsunit/math-sqrt.js (right):

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/math-sqrt.js#newco...
test/mjsunit/math-sqrt.js:33: // Math.pow(-0, 0.5) must be zero, but
Math.sqrt(-0) is -0.
I think this should be converted from a comment into a test.

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mjsunit.js
File test/mjsunit/mjsunit.js (right):

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mjsunit.js#newcode44
test/mjsunit/mjsunit.js:44: function MJSToString(value) {
You should adopt the same naming convention as the surrounding code or else
change the surrounding code (MjsToString, see MjsUnitAssertionError just above,
and I think it should properly be MjsUnitToString---"mini JSUnit" is the testing
framework, I don't know what "mini JS" is).

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mjsunit.js#newcode82
test/mjsunit/mjsunit.js:82: var start;
I can't figure out why this function is so convoluted.

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js
File test/mjsunit/mul-exhaustive.js (right):

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js#...
test/mjsunit/mul-exhaustive.js:33: // Test multiplication where both operands
are variables.
It's hard to reverse engineer what the the new parts of this test are doing. 
The cryptic identifier names don't help.

This comment does not help at all, because "-x" is not a variable.  Please just
drop this comment.

Please rename "a" to "expected".

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js#...
test/mjsunit/mul-exhaustive.js:43: // Test multiplication where either or both
operands are constant.
"Constant" is ambiguous (especially if contrasted with "variable", since we have
const in the language).  Use "literals".

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js#...
test/mjsunit/mul-exhaustive.js:46: var xs = String(x);
It might be more straightforward to skip converting both x and y and both their
negations to strings by defining a helper

function stringify(n) { return (1 / n === -Infinity) ? '-0' : String(n); }

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js#...
test/mjsunit/mul-exhaustive.js:53: function fs(a, x, y, xs, ys) {
Here I think you could probably find a way to fold the earlier asserts into this
function.  You could also convert x and y to strings on demand.  I'm thinking
something like:

function subtest(expected, x, y) {
  assertEquals(expected, x * y);
  assertEquals(expected, eval(stringify(x) + '*y'));
  assertEquals(expected, eval('x*' + stringify(y));
  assertEquals(expected, eval(stringify(x) + '*' + stringify(y));
}
subtest(expected, x, y);
subtest(-expected, -x, -y);
subtext(-expected, x, -y);
// etc.

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js#...
test/mjsunit/mul-exhaustive.js:55: for (var i = 0; i < 1000; i++)
assertEquals(a, xcyv(y));
I don't understand the purpose of the loop.  If it's to hit the optimizing
compiler, I think you'll need a bigger constant.

[Lasse Reichstein, 2011-03-21 13:17:58]

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/math-sqrt.js
File test/mjsunit/math-sqrt.js (right):

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/math-sqrt.js#newco...
test/mjsunit/math-sqrt.js:33: // Math.pow(-0, 0.5) must be zero, but
Math.sqrt(-0) is -0.
Done.

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mjsunit.js
File test/mjsunit/mjsunit.js (right):

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mjsunit.js#newcode44
test/mjsunit/mjsunit.js:44: function MJSToString(value) {
Renamed to MjsUnitToString.

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mjsunit.js#newcode82
test/mjsunit/mjsunit.js:82: var start;
And it doesn't have to be.
I added the second case for message for cases where the type of actual differs
from expected, but with MjsUnitToString, it doesn't matter as much, since it
prints strings with quotes around (so we don't get two different values that
turns to the same string when showing).

I'll try to make it simpler.

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js
File test/mjsunit/mul-exhaustive.js (right):

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js#...
test/mjsunit/mul-exhaustive.js:33: // Test multiplication where both operands
are variables.
Done.

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js#...
test/mjsunit/mul-exhaustive.js:43: // Test multiplication where either or both
operands are constant.
Done.

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js#...
test/mjsunit/mul-exhaustive.js:46: var xs = String(x);
Done.

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js#...
test/mjsunit/mul-exhaustive.js:53: function fs(a, x, y, xs, ys) {
Much better, yes.
I'm a little afraid of using eval if we want the code to be optimized. It will
cause context allocated arguments.
I'll add a wrapper for Function to make it more readable.

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js#...
test/mjsunit/mul-exhaustive.js:55: for (var i = 0; i < 1000; i++)
assertEquals(a, xcyv(y));
If anything, I should check which constant is necessary to make it optimize with
--opt-eagerly.
The problem is that this test runs rather long, so if I increase it to 10000, it
does optimize more anonymous functions (using --always-opt and --opt-eagerly),
but not all of them anyway. If I go to 100000, it starts to take several
minutes. I'll use 10000 for now, but suggestions are welcome.

[Kevin Millikin (Chromium), 2011-03-21 13:37:35]

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mjsunit.js
File test/mjsunit/mjsunit.js (right):

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mjsunit.js#newcode82
test/mjsunit/mjsunit.js:82: var start;
On 2011/03/21 13:17:58, Lasse Reichstein wrote:
> And it doesn't have to be.
> I added the second case for message for cases where the type of actual differs
> from expected, but with MjsUnitToString, it doesn't matter as much, since it
> prints strings with quotes around (so we don't get two different values that
> turns to the same string when showing).
> 
> I'll try to make it simpler.

Here's where we need printf :)

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js
File test/mjsunit/mul-exhaustive.js (right):

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js#...
test/mjsunit/mul-exhaustive.js:55: for (var i = 0; i < 1000; i++)
assertEquals(a, xcyv(y));
On 2011/03/21 13:17:58, Lasse Reichstein wrote:
> If anything, I should check which constant is necessary to make it optimize
with
> --opt-eagerly.
> The problem is that this test runs rather long, so if I increase it to 10000,
it
> does optimize more anonymous functions (using --always-opt and --opt-eagerly),
> but not all of them anyway. If I go to 100000, it starts to take several
> minutes. I'll use 10000 for now, but suggestions are welcome.

I think I'd just leave it out of this test.

We *do* need better tests of the optimizing compiler, but this way is so
indirect and not guaranteed to always test what we want it to test.

We should try to come up with a way to write unit tests for the individual
instructions instead.

[Lasse Reichstein, 2011-03-21 14:18:26]

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mjsunit.js
File test/mjsunit/mjsunit.js (right):

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mjsunit.js#newcode82
test/mjsunit/mjsunit.js:82: var start;
Indeed! :)

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js
File test/mjsunit/mul-exhaustive.js (right):

http://codereview.chromium.org/6688062/diff/1/test/mjsunit/mul-exhaustive.js#...
test/mjsunit/mul-exhaustive.js:55: for (var i = 0; i < 1000; i++)
assertEquals(a, xcyv(y));
Will do. Simplicity FTW.