Re: [Tarantool-patches] [PATCH luajit v4 3/3] Cleanup math function compilation and fix inconsistencies.

[Sergey Kaplun via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Maxim!

Thanks for the patch!

Please consider my comments below.

On 30.03.22, Maxim Kokryashkin wrote:
> This patch changes 'math_unary', `math_htrig` and `math_atrig`
> to `math_call` for math functions compilation. Now all of
> math functions in IRs are called with `CALLN` instead of
> `FPMATH` (`ATAN2` for `math.atan2`).

It should be mentioned that IR ATAN2 is removed in this patch.
Also, we can drop some words about new fold optimizations.

Side note: Did Mike means by inconsistencies in organization of code
structure?  Or it was inconsistent behaviour?

> 
> Part of tarantool/tarantool#6163
> ---
>  src/lib_math.c                                | 22 +++----
>  src/lj_asm.c                                  |  6 --
>  src/lj_asm_arm.h                              |  1 -
>  src/lj_asm_arm64.h                            |  1 -
>  src/lj_asm_x86.h                              |  2 -
>  src/lj_ffrecord.c                             | 19 +-----
>  src/lj_ir.h                                   |  4 +-
>  src/lj_ircall.h                               | 14 +++--
>  src/lj_opt_fold.c                             | 25 +++++++-
>  src/lj_opt_split.c                            |  3 -
>  src/lj_target_x86.h                           |  6 --
>  src/lj_vmmath.c                               |  6 --
>  .../gh-6163-jit-min-max.test.lua              | 63 ++++++++++++++++++-
>  13 files changed, 109 insertions(+), 63 deletions(-)
> 
> diff --git a/src/lib_math.c b/src/lib_math.c
> index 4e6d2458..601655cd 100644
> --- a/src/lib_math.c
> +++ b/src/lib_math.c

<snipped>

> diff --git a/src/lj_asm.c b/src/lj_asm.c
> index 10e5872b..1a7fb0c8 100644
> --- a/src/lj_asm.c
> +++ b/src/lj_asm.c

<snipped>

> diff --git a/src/lj_asm_arm.h b/src/lj_asm_arm.h
> index 84ab06c0..6ae6e2f2 100644
> --- a/src/lj_asm_arm.h
> +++ b/src/lj_asm_arm.h

<snipped>

> diff --git a/src/lj_asm_arm64.h b/src/lj_asm_arm64.h
> index e93e29e3..6a115f2a 100644
> --- a/src/lj_asm_arm64.h
> +++ b/src/lj_asm_arm64.h

<snipped>

> diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h
> index 767bf6f3..f75af8a4 100644
> --- a/src/lj_asm_x86.h
> +++ b/src/lj_asm_x86.h

<snipped>

> diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c
> index ac9c9ba1..649ac705 100644
> --- a/src/lj_ffrecord.c
> +++ b/src/lj_ffrecord.c

<snipped>

> diff --git a/src/lj_ir.h b/src/lj_ir.h
> index 3059bf65..4bad47ed 100644
> --- a/src/lj_ir.h
> +++ b/src/lj_ir.h

<snipped>

> diff --git a/src/lj_ircall.h b/src/lj_ircall.h
> index 973c36e6..aa06b273 100644
> --- a/src/lj_ircall.h
> +++ b/src/lj_ircall.h

<snipped>

> diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
> index 29b760b4..2c9122c5 100644
> --- a/src/lj_opt_fold.c
> +++ b/src/lj_opt_fold.c
> @@ -173,7 +173,6 @@ LJFOLD(ADD KNUM KNUM)
>  LJFOLD(SUB KNUM KNUM)
>  LJFOLD(MUL KNUM KNUM)
>  LJFOLD(DIV KNUM KNUM)
> -LJFOLD(ATAN2 KNUM KNUM)
>  LJFOLD(LDEXP KNUM KNUM)
>  LJFOLD(MIN KNUM KNUM)
>  LJFOLD(MAX KNUM KNUM)
> @@ -213,6 +212,30 @@ LJFOLDF(kfold_fpmath)
>    return lj_ir_knum(J, y);
>  }
>  
> +LJFOLD(CALLN KNUM any)
> +LJFOLDF(kfold_fpcall1)
> +{
> +  const CCallInfo *ci = &lj_ir_callinfo[fins->op2];
> +  if (CCI_TYPE(ci) == IRT_NUM) {
> +    double y = ((double (*)(double))ci->func)(knumleft);
> +    return lj_ir_knum(J, y);
> +  }
> +  return NEXTFOLD;
> +}
> +
> +LJFOLD(CALLN CARG IRCALL_atan2)
> +LJFOLDF(kfold_fpcall2)
> +{
> +  if (irref_isk(fleft->op1) && irref_isk(fleft->op2)) {
> +    const CCallInfo *ci = &lj_ir_callinfo[fins->op2];
> +    double a = ir_knum(IR(fleft->op1))->n;
> +    double b = ir_knum(IR(fleft->op2))->n;
> +    double y = ((double (*)(double, double))ci->func)(a, b);
> +    return lj_ir_knum(J, y);
> +  }
> +  return NEXTFOLD;
> +}
> +
>  LJFOLD(POW KNUM KINT)
>  LJFOLDF(kfold_numpow)
>  {
> diff --git a/src/lj_opt_split.c b/src/lj_opt_split.c
> index fc935204..c0788106 100644
> --- a/src/lj_opt_split.c
> +++ b/src/lj_opt_split.c

<snipped>

> diff --git a/src/lj_target_x86.h b/src/lj_target_x86.h
> index 356f7924..194f8e70 100644
> --- a/src/lj_target_x86.h
> +++ b/src/lj_target_x86.h
> @@ -228,16 +228,10 @@ typedef enum {

<snipped>

> diff --git a/src/lj_vmmath.c b/src/lj_vmmath.c
> index 08ccf467..ae4e0f15 100644
> --- a/src/lj_vmmath.c
> +++ b/src/lj_vmmath.c

<snipped>

> diff --git a/test/tarantool-tests/gh-6163-jit-min-max.test.lua b/test/tarantool-tests/gh-6163-jit-min-max.test.lua
> index 4594b968..f9e69842 100644
> --- a/test/tarantool-tests/gh-6163-jit-min-max.test.lua
> +++ b/test/tarantool-tests/gh-6163-jit-min-max.test.lua
> @@ -1,10 +1,37 @@


These new tests are not related to min/max functions, so they should be
moved to the separate test file.

Also, I'm not sure about the tests themselves -- what are they checking?
Maybe we should check at lease fold optimizations instead?

>  local tap = require('tap')
>  local jutil = require('jit.util')
> +local bit = require('bit')
> +local vmdef = require('jit.vmdef')
> +
> +local shr = bit.rshift
> +local band = bit.band
> +local sub = string.sub
> +local traceinfo = jutil.traceinfo
> +local traceir = jutil.traceir
> +
> +local function check_ir(tr, funcnm)
> +  local info = traceinfo(tr)
> +  assert(info ~= nil)
> +
> +  local nins = info.nins
> +  local irnames = vmdef.irnames
> +
> +  for ins = 1, nins do
> +    local m, ot, _, op2, _ = traceir(tr, ins)
> +    local oidx = 6 * shr(ot, 8)
> +    local op = sub(irnames, oidx + 1, oidx + 6)
> +    if sub(op, 1, 5) == "CALLN" and
> +      band(m, 3 * 4) == 4 and
> +      vmdef.ircall[op2] == funcnm then return true end
> +  end
> +  return false
> +end

I suggest to move this function to the <utils.lua> and change its
prototype to the following:

| function M.hasir(traceno, ir, irnum)

Where irnum is an optional field to check that ir has the corresponding
number (may be it should be some callback instead if IR is found).

> +
>  jit.off()
>  jit.flush()
>  
>  local test = tap.test("gh-6163-jit-min-max")
> -test:plan(6)
> +test:plan(18)
>  
>  -- `math.min`/`math.max` success with no args.
>  local pcall = pcall
> @@ -32,4 +59,38 @@ test:ok(tr2_info ~= nil)
>  test:ok(tr1_info.link == 2)
>  test:ok(tr1_info.linktype == "stitch")
>  
> +jit.off()
> +jit.flush()
> +jit.on()
> +
> +-- All math functions should appear as `CALLN` in IRs.
> +for _=1,3 do
> +  math.sin(1)
> +  math.cos(1)
> +  math.tan(1)
> +  math.exp(1)
> +  math.log10(1)
> +  math.asin(0)
> +  math.acos(0)
> +  math.atan(1)
> +  math.atan2(2, 1)
> +  math.sinh(1)
> +  math.cosh(1)
> +  math.tanh(1)
> +end
> +jit.off()
> +
> +test:ok(check_ir(1, "sin") == true)
> +test:ok(check_ir(1, "cos") == true)
> +test:ok(check_ir(1, "tan") == true)
> +test:ok(check_ir(1, "exp") == true)
> +test:ok(check_ir(1, "log10") == true)
> +test:ok(check_ir(1, "asin") == true)
> +test:ok(check_ir(1, "acos") == true)
> +test:ok(check_ir(1, "atan") == true)
> +test:ok(check_ir(1, "atan2") == true)
> +test:ok(check_ir(1, "sinh") == true)
> +test:ok(check_ir(1, "cosh") == true)
> +test:ok(check_ir(1, "tanh") == true)
> +
>  os.exit(test:check() and 0 or 1)
> -- 
> 2.35.1
> 

-- 
Best regards,
Sergey Kaplun