892702 - Range analysis code needs actual unit tests

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Right now the range analysis code is only tested indirectly, through jit-tests, jstests, etc.  But this problem is narrowly scoped enough that we can, and should, test the range analysis code directly at the C++ level, in jsapi-tests (that use purely internal headers, but it's okay for JSAPI tests to do that).  It's a whole lot easier constructing debugging a jsapi-test of range analysis stuff, than it is of JS code that happens to invoke the range analysis code in the same way.  (Also, JS might constant-fold and do other things such that it's difficult to impossible to invoke range analysis with the precise desired inputs, which might hide bugs thought impossible.)

There's a little messiness here in that Range is TempObject right now, but I imagine throwing some C++ templates at the problem, and having (say) |template<class Base> class RangeImpl : public Base| with |typedef RangeImpl<TempObject> Range;| or something would solve things pretty nicely.

Comment 1

[Dan Gohman [:sunfish]]

js/src/jsapi-tests/testJitRValueAlloc.cpp is now an example of a JIT test in jsapi-tests.

Comment 2

[Nicolas B. Pierron [:nbp]]

This bug is not a good first bug, as it requires a bit of knowledge about Range Analysis.  I would recommend to have a look at the work being done in order to improve our documentation on Range Analysis (Bug 1028274)

The goal of adding unit-test for Range Analysis is first to test all functions which are made on Range [1], and ensure that if we provide any value within the input range, then the result is within the output range.

Among interesting values which are interesting in terms of boundaries of inputs / outputs ranges, we have:

  ±Inf
  ±Math.pow(2,52) (± 1)
  ±Math.pow(2,32) (± 1)
  ±Math.pow(2,31) (± 1)
  ±Math.pow(2,n) (± 0.5) [n < 32]
  ±0

So there is a lot of potential test cases to make and a lot of values to iterate over.

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/RangeAnalysis.h?from=RangeAnalysis.h#398-418

Comment 3

[Dan Gohman [:sunfish]]

Bug 1036037 added js/src/jsapi-tests/testJitFoldsTo.cpp, which is an example of a JIT test in jsapi-tests that builds a MIRGraph.

Comment 4

[Dan Gohman [:sunfish]]

Attachment: range-unit-test.patch

This test redoes the test from bug 1094052 to be more unit-test-like, calling the Range class functions directly, rather than going through all the trouble of building a MIRGraph. I think this is a good example of what we want unit tests for the Range class to look like.

It also adds a test for bug 1093356, which is a good example of a testcase which does need to build a full MIRGraph.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8519105 [details] [diff] [review]
range-unit-test.patch

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

::: js/src/jsapi-tests/testJitRangeAnalysis.cpp
@@ +52,5 @@
> +    Range *zero = Range::NewDoubleSingletonRange(func.alloc, 0.0);
> +    Range *half = Range::NewDoubleSingletonRange(func.alloc, 0.5);
> +    Range *one = Range::NewDoubleSingletonRange(func.alloc, 1.0);
> +    Range *threehalves = Range::NewDoubleSingletonRange(func.alloc, 1.5);
> +    Range *inf = Range::NewDoubleSingletonRange(func.alloc, mozilla::PositiveInfinity<double>());

nit: I think that using variables with identical length would improve the readability of this test:

  _nan
  ninf
  n1_5
  n1_0
  n0_5
  n0_0
  p0_0
  p0_5
  p1_0
  p1_5
  pinf

Comment 6

[Dan Gohman [:sunfish]]

Done. https://hg.mozilla.org/integration/mozilla-inbound/rev/b27100f83983

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b27100f83983