Re: [Tarantool-patches] [PATCH luajit 1/5] test: introduce `samevalues()` TAP checker

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Sergey!
Thanks for the patch!
LGTM, except for a few comments below.

On Tue, Aug 15, 2023 at 12:36:27PM +0300, Sergey Kaplun wrote:
> The introduced `samevalues()` helper checks that values in range from
Typo: s/in range/in the range/
> 1, to `table.maxn()` of the given table are exactly the same. It may be
> usefull for test consistency of JIT and VM behaviour. Originally, the
Typo: s/usefull for test/useful to test the/
> `arr_is_consistent()` function was introduced in the
> <tarantool-tests/gh-6163-min-max.test.lua>. `samevalues()` has the same
> functionallity (except usage of `table.maxn()` instead `#` operator to
Typo: s/functionallity/functionality/
Typo: s/except/except for the/
Typo: s/instead/instead of the/
> be sure, that the table we check isn't a sparse array).
> ---
>  test/tarantool-tests/gh-6163-min-max.test.lua | 52 ++++++++-----------
>  test/tarantool-tests/tap.lua                  | 14 +++++
>  2 files changed, 37 insertions(+), 29 deletions(-)
> 
> diff --git a/test/tarantool-tests/gh-6163-min-max.test.lua b/test/tarantool-tests/gh-6163-min-max.test.lua
> index 63437955..4bc6155c 100644
> --- a/test/tarantool-tests/gh-6163-min-max.test.lua
> +++ b/test/tarantool-tests/gh-6163-min-max.test.lua
> @@ -2,25 +2,17 @@ local tap = require('tap')
>  local test = tap.test('gh-6163-jit-min-max'):skipcond({
>    ['Test requires JIT enabled'] = not jit.status(),
>  })
> +
>  local x86_64 = jit.arch == 'x86' or jit.arch == 'x64'
> +-- XXX: table to use for dummy check for some inconsistent results
> +-- on the x86/64 architecture.
> +local DUMMY_TAB = {}
> +
>  test:plan(18)
>  --
>  -- gh-6163: math.min/math.max inconsistencies.
>  --
>  
> -local function isnan(x)
> -    return x ~= x
> -end
> -
> -local function array_is_consistent(res)
> -  for i = 1, #res - 1 do
> -    if res[i] ~= res[i + 1] and not (isnan(res[i]) and isnan(res[i + 1])) then
> -      return false
> -    end
> -  end
> -  return true
> -end
> -
>  -- This function creates dirty values on the Lua stack.
>  -- The latter of them is going to be treated as an
>  -- argument by the `math.min/math.max`.
> @@ -91,14 +83,14 @@ for k = 1, 4 do
>      result[k] = min(min(x, nan), x)
>  end
>  -- expected: 1 1 1 1
> -test:ok(array_is_consistent(result), 'math.min: reassoc_dup')
> +test:samevalues(result, 'math.min: reassoc_dup')
>  
>  result = {}
>  for k = 1, 4 do
>      result[k] = max(max(x, nan), x)
>  end
>  -- expected: 1 1 1 1
> -test:ok(array_is_consistent(result), 'math.max: reassoc_dup')
> +test:samevalues(result, 'math.max: reassoc_dup')
>  
>  -- If one gets the expression like `math.min(x, math.min(x, nan))`,
>  -- and the `comm_dup` optimization is applied, it results in the
> @@ -120,7 +112,7 @@ for k = 1, 4 do
>  end
>  -- FIXME: results are still inconsistent for the x86/64 architecture.
>  -- expected: nan nan nan nan
> -test:ok(array_is_consistent(result) or x86_64, 'math.min: comm_dup_minmax')
> +test:samevalues(x86_64 and DUMMY_TAB or result, 'math.min: comm_dup_minmax')
>  
>  result = {}
>  for k = 1, 4 do
> @@ -128,7 +120,7 @@ for k = 1, 4 do
>  end
>  -- FIXME: results are still inconsistent for the x86/64 architecture.
>  -- expected: nan nan nan nan
> -test:ok(array_is_consistent(result) or x86_64, 'math.max: comm_dup_minmax')
> +test:samevalues(x86_64 and DUMMY_TAB or result, 'math.max: comm_dup_minmax')
>  
>  -- The following optimization should be disabled:
>  -- (x o k1) o k2 ==> x o (k1 o k2)
> @@ -139,49 +131,49 @@ for k = 1, 4 do
>      result[k] = min(min(x, 0/0), 1.3)
>  end
>  -- expected: 1.3 1.3 1.3 1.3
> -test:ok(array_is_consistent(result), 'math.min: reassoc_minmax_k')
> +test:samevalues(result, 'math.min: reassoc_minmax_k')
>  
>  result = {}
>  for k = 1, 4 do
>      result[k] = max(max(x, 0/0), 1.1)
>  end
>  -- expected: 1.1 1.1 1.1 1.1
> -test:ok(array_is_consistent(result), 'math.max: reassoc_minmax_k')
> +test:samevalues(result, 'math.max: reassoc_minmax_k')
>  
>  result = {}
>  for k = 1, 4 do
>    result[k] = min(max(nan, 1), 1)
>  end
>  -- expected: 1 1 1 1
> -test:ok(array_is_consistent(result), 'min-max-case1: reassoc_minmax_left')
> +test:samevalues(result, 'min-max-case1: reassoc_minmax_left')
>  
>  result = {}
>  for k = 1, 4 do
>    result[k] = min(max(1, nan), 1)
>  end
>  -- expected: 1 1 1 1
> -test:ok(array_is_consistent(result), 'min-max-case2: reassoc_minmax_left')
> +test:samevalues(result, 'min-max-case2: reassoc_minmax_left')
>  
>  result = {}
>  for k = 1, 4 do
>    result[k] = max(min(nan, 1), 1)
>  end
>  -- expected: 1 1 1 1
> -test:ok(array_is_consistent(result), 'max-min-case1: reassoc_minmax_left')
> +test:samevalues(result, 'max-min-case1: reassoc_minmax_left')
>  
>  result = {}
>  for k = 1, 4 do
>    result[k] = max(min(1, nan), 1)
>  end
>  -- expected: 1 1 1 1
> -test:ok(array_is_consistent(result), 'max-min-case2: reassoc_minmax_left')
> +test:samevalues(result, 'max-min-case2: reassoc_minmax_left')
>  
>  result = {}
>  for k = 1, 4 do
>    result[k] = min(1, max(nan, 1))
>  end
>  -- expected: 1 1 1 1
> -test:ok(array_is_consistent(result), 'min-max-case1: reassoc_minmax_right')
> +test:samevalues(result, 'min-max-case1: reassoc_minmax_right')
>  
>  result = {}
>  for k = 1, 4 do
> @@ -189,14 +181,15 @@ for k = 1, 4 do
>  end
>  -- FIXME: results are still inconsistent for the x86/64 architecture.
>  -- expected: nan nan nan nan
> -test:ok(array_is_consistent(result) or x86_64, 'min-max-case2: reassoc_minmax_right')
> +test:samevalues(x86_64 and DUMMY_TAB or result,
> +                'min-max-case2: reassoc_minmax_right')

Side note: this skipcond looks complex, but I can't come up with an alternative
better than altering the TAP-plan, which is even worse option...
>  
>  result = {}
>  for k = 1, 4 do
>    result[k] = max(1, min(nan, 1))
>  end
>  -- expected: 1 1 1 1
> -test:ok(array_is_consistent(result), 'max-min-case1: reassoc_minmax_right')
> +test:samevalues(result, 'max-min-case1: reassoc_minmax_right')
>  
>  result = {}
>  for k = 1, 4 do
> @@ -204,7 +197,8 @@ for k = 1, 4 do
>  end
>  -- FIXME: results are still inconsistent for the x86/64 architecture.
>  -- expected: nan nan nan nan
> -test:ok(array_is_consistent(result) or x86_64, 'max-min-case2: reassoc_minmax_right')
> +test:samevalues(x86_64 and DUMMY_TAB or result,
> +                'max-min-case2: reassoc_minmax_right')
>  
>  -- XXX: If we look into the disassembled code of `lj_vm_foldarith()`
>  -- we can see the following:
> @@ -253,13 +247,13 @@ for k = 1, 4 do
>    result[k] = min(min(7.1, 0/0), 1.1)
>  end
>  -- expected: 1.1 1.1 1.1 1.1
> -test:ok(array_is_consistent(result), 'min: fold_kfold_numarith')
> +test:samevalues(result, 'min: fold_kfold_numarith')
>  
>  result = {}
>  for k = 1, 4 do
>    result[k] = max(max(7.1, 0/0), 1.1)
>  end
>  -- expected: 1.1 1.1 1.1 1.1
> -test:ok(array_is_consistent(result), 'max: fold_kfold_numarith')
> +test:samevalues(result, 'max: fold_kfold_numarith')
>  
>  test:done(true)
> diff --git a/test/tarantool-tests/tap.lua b/test/tarantool-tests/tap.lua
> index 8559ee52..af1d4b20 100644
> --- a/test/tarantool-tests/tap.lua
> +++ b/test/tarantool-tests/tap.lua
> @@ -254,6 +254,19 @@ local function iscdata(test, v, ctype, message, extra)
>    return ok(test, ffi.istype(ctype, v), message, extra)
>  end
>  
> +local function isnan(v)
> +  return v ~= v
> +end
> +
> +local function samevalues(test, got, message, extra)
> +  for i = 1, table.maxn(got) - 1 do
> +    if got[i] ~= got[i + 1] and not (isnan(got[i]) and isnan(got[i + 1])) then
> +      return fail(test, message, extra)
> +    end
> +  end
> +  return ok(test, true, message, extra)
> +end
> +
>  local test_mt
>  
>  local function new(parent, name, fun, ...)
> @@ -372,6 +385,7 @@ test_mt = {
>      isudata    = isudata,
>      iscdata    = iscdata,
>      is_deeply  = is_deeply,
> +    samevalues = samevalues,
>      like       = like,
>      unlike     = unlike,
>    }
> -- 
> 2.41.0
>