* [Tarantool-patches] [PATCH luajit v4 0/3] fix math.min/max inconsistencies
@ 2022-03-29 23:48 Maxim Kokryashkin via Tarantool-patches
2022-03-29 23:48 ` [Tarantool-patches] [PATCH luajit v4 1/3] Fix math.min()/math.max() inconsistencies Maxim Kokryashkin via Tarantool-patches
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2022-03-29 23:48 UTC (permalink / raw)
To: tarantool-patches, imun, skaplun
Changes in v4:
- Added tests for all patches.
- Added more informative description for the last patch.
GitHub branch: https://github.com/tarantool/luajit/tree/fckxorg/gh-6163-min-max
Issue: https://github.com/tarantool/tarantool/issues/6163
Maxim Kokryashkin (1):
Cleanup math function compilation and fix inconsistencies.
Mike Pall (2):
Fix math.min()/math.max() inconsistencies.
Don't compile math.modf() anymore.
src/lib_math.c | 24 ++---
src/lj_asm.c | 6 --
src/lj_asm_arm.h | 7 +-
src/lj_asm_arm64.h | 7 +-
src/lj_asm_x86.h | 2 -
src/lj_ffrecord.c | 35 +------
src/lj_ir.h | 4 +-
src/lj_ircall.h | 14 ++-
src/lj_opt_fold.c | 78 ++++++++-------
src/lj_opt_split.c | 3 -
src/lj_target_x86.h | 6 --
src/lj_vmmath.c | 10 +-
src/vm_arm.dasc | 4 +-
src/vm_arm64.dasc | 4 +-
src/vm_x64.dasc | 2 +-
src/vm_x86.dasc | 2 +-
.../gh-6163-jit-min-max.test.lua | 96 +++++++++++++++++++
17 files changed, 177 insertions(+), 127 deletions(-)
create mode 100644 test/tarantool-tests/gh-6163-jit-min-max.test.lua
--
2.35.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Tarantool-patches] [PATCH luajit v4 1/3] Fix math.min()/math.max() inconsistencies.
2022-03-29 23:48 [Tarantool-patches] [PATCH luajit v4 0/3] fix math.min/max inconsistencies Maxim Kokryashkin via Tarantool-patches
@ 2022-03-29 23:48 ` Maxim Kokryashkin via Tarantool-patches
2022-04-14 8:55 ` Sergey Kaplun via Tarantool-patches
2022-03-29 23:48 ` [Tarantool-patches] [PATCH luajit v4 2/3] Don't compile math.modf() anymore Maxim Kokryashkin via Tarantool-patches
2022-03-29 23:48 ` [Tarantool-patches] [PATCH luajit v4 3/3] Cleanup math function compilation and fix inconsistencies Maxim Kokryashkin via Tarantool-patches
2 siblings, 1 reply; 7+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2022-03-29 23:48 UTC (permalink / raw)
To: tarantool-patches, imun, skaplun
From: Mike Pall <mike>
math.min()/math.max() could produce different results when compiled.
Previously, these functions did not check for the number of arguments,
which in some situations led to incorrect behavior.
Consider this example:
```
jit.opt.start(0, "hotloop=1")
local r, msg = pcall(function () math.min() end)
r, msg = pcall(function () math.min() end)
```
Before this patch, this snippet of code completed successfully, although it
was not supposed to, since there were no args to math.min().
This patch adds check for the number of arguments for jit-compiled
version of math.min/max, and it also adds the corresponding test case for
the mentioned issue.
Part of tarantool/tarantool#6163
Part of tarantool/tarantool#6548
---
src/lj_asm_arm.h | 6 +--
src/lj_asm_arm64.h | 6 +--
src/lj_opt_fold.c | 53 +++++++------------
src/lj_vmmath.c | 4 +-
src/vm_arm.dasc | 4 +-
src/vm_arm64.dasc | 4 +-
src/vm_x64.dasc | 2 +-
src/vm_x86.dasc | 2 +-
.../gh-6163-jit-min-max.test.lua | 20 +++++++
9 files changed, 53 insertions(+), 48 deletions(-)
create mode 100644 test/tarantool-tests/gh-6163-jit-min-max.test.lua
diff --git a/src/lj_asm_arm.h b/src/lj_asm_arm.h
index 4fd08b9e..84ab06c0 100644
--- a/src/lj_asm_arm.h
+++ b/src/lj_asm_arm.h
@@ -1664,8 +1664,8 @@ static void asm_min_max(ASMState *as, IRIns *ir, int cc, int fcc)
asm_intmin_max(as, ir, cc);
}
-#define asm_min(as, ir) asm_min_max(as, ir, CC_GT, CC_HI)
-#define asm_max(as, ir) asm_min_max(as, ir, CC_LT, CC_LO)
+#define asm_min(as, ir) asm_min_max(as, ir, CC_GT, CC_PL)
+#define asm_max(as, ir) asm_min_max(as, ir, CC_LT, CC_LE)
/* -- Comparisons --------------------------------------------------------- */
@@ -1857,7 +1857,7 @@ static void asm_hiop(ASMState *as, IRIns *ir)
} else if ((ir-1)->o == IR_MIN || (ir-1)->o == IR_MAX) {
as->curins--; /* Always skip the loword min/max. */
if (uselo || usehi)
- asm_sfpmin_max(as, ir-1, (ir-1)->o == IR_MIN ? CC_HI : CC_LO);
+ asm_sfpmin_max(as, ir-1, (ir-1)->o == IR_MIN ? CC_PL : CC_LE);
return;
#elif LJ_HASFFI
} else if ((ir-1)->o == IR_CONV) {
diff --git a/src/lj_asm_arm64.h b/src/lj_asm_arm64.h
index cc8c0c9d..e93e29e3 100644
--- a/src/lj_asm_arm64.h
+++ b/src/lj_asm_arm64.h
@@ -1594,7 +1594,7 @@ static void asm_fpmin_max(ASMState *as, IRIns *ir, A64CC fcc)
Reg dest = (ra_dest(as, ir, RSET_FPR) & 31);
Reg right, left = ra_alloc2(as, ir, RSET_FPR);
right = ((left >> 8) & 31); left &= 31;
- emit_dnm(as, A64I_FCSELd | A64F_CC(fcc), dest, left, right);
+ emit_dnm(as, A64I_FCSELd | A64F_CC(fcc), dest, right, left);
emit_nm(as, A64I_FCMPd, left, right);
}
@@ -1606,8 +1606,8 @@ static void asm_min_max(ASMState *as, IRIns *ir, A64CC cc, A64CC fcc)
asm_intmin_max(as, ir, cc);
}
-#define asm_max(as, ir) asm_min_max(as, ir, CC_GT, CC_HI)
-#define asm_min(as, ir) asm_min_max(as, ir, CC_LT, CC_LO)
+#define asm_min(as, ir) asm_min_max(as, ir, CC_LT, CC_PL)
+#define asm_max(as, ir) asm_min_max(as, ir, CC_GT, CC_LE)
/* -- Comparisons --------------------------------------------------------- */
diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
index 3c508062..29b760b4 100644
--- a/src/lj_opt_fold.c
+++ b/src/lj_opt_fold.c
@@ -1774,8 +1774,6 @@ LJFOLDF(reassoc_intarith_k64)
#endif
}
-LJFOLD(MIN MIN any)
-LJFOLD(MAX MAX any)
LJFOLD(BAND BAND any)
LJFOLD(BOR BOR any)
LJFOLDF(reassoc_dup)
@@ -1785,6 +1783,15 @@ LJFOLDF(reassoc_dup)
return NEXTFOLD;
}
+LJFOLD(MIN MIN any)
+LJFOLD(MAX MAX any)
+LJFOLDF(reassoc_dup_minmax)
+{
+ if (fins->op2 == fleft->op2)
+ return LEFTFOLD; /* (a o b) o b ==> a o b */
+ return NEXTFOLD;
+}
+
LJFOLD(BXOR BXOR any)
LJFOLDF(reassoc_bxor)
{
@@ -1823,23 +1830,12 @@ LJFOLDF(reassoc_shift)
return NEXTFOLD;
}
-LJFOLD(MIN MIN KNUM)
-LJFOLD(MAX MAX KNUM)
LJFOLD(MIN MIN KINT)
LJFOLD(MAX MAX KINT)
LJFOLDF(reassoc_minmax_k)
{
IRIns *irk = IR(fleft->op2);
- if (irk->o == IR_KNUM) {
- lua_Number a = ir_knum(irk)->n;
- lua_Number y = lj_vm_foldarith(a, knumright, fins->o - IR_ADD);
- if (a == y) /* (x o k1) o k2 ==> x o k1, if (k1 o k2) == k1. */
- return LEFTFOLD;
- PHIBARRIER(fleft);
- fins->op1 = fleft->op1;
- fins->op2 = (IRRef1)lj_ir_knum(J, y);
- return RETRYFOLD; /* (x o k1) o k2 ==> x o (k1 o k2) */
- } else if (irk->o == IR_KINT) {
+ if (irk->o == IR_KINT) {
int32_t a = irk->i;
int32_t y = kfold_intop(a, fright->i, fins->o);
if (a == y) /* (x o k1) o k2 ==> x o k1, if (k1 o k2) == k1. */
@@ -1852,24 +1848,6 @@ LJFOLDF(reassoc_minmax_k)
return NEXTFOLD;
}
-LJFOLD(MIN MAX any)
-LJFOLD(MAX MIN any)
-LJFOLDF(reassoc_minmax_left)
-{
- if (fins->op2 == fleft->op1 || fins->op2 == fleft->op2)
- return RIGHTFOLD; /* (b o1 a) o2 b ==> b; (a o1 b) o2 b ==> b */
- return NEXTFOLD;
-}
-
-LJFOLD(MIN any MAX)
-LJFOLD(MAX any MIN)
-LJFOLDF(reassoc_minmax_right)
-{
- if (fins->op1 == fright->op1 || fins->op1 == fright->op2)
- return LEFTFOLD; /* a o2 (a o1 b) ==> a; a o2 (b o1 a) ==> a */
- return NEXTFOLD;
-}
-
/* -- Array bounds check elimination -------------------------------------- */
/* Eliminate ABC across PHIs to handle t[i-1] forwarding case.
@@ -1995,8 +1973,6 @@ LJFOLDF(comm_comp)
LJFOLD(BAND any any)
LJFOLD(BOR any any)
-LJFOLD(MIN any any)
-LJFOLD(MAX any any)
LJFOLDF(comm_dup)
{
if (fins->op1 == fins->op2) /* x o x ==> x */
@@ -2004,6 +1980,15 @@ LJFOLDF(comm_dup)
return fold_comm_swap(J);
}
+LJFOLD(MIN any any)
+LJFOLD(MAX any any)
+LJFOLDF(comm_dup_minmax)
+{
+ if (fins->op1 == fins->op2) /* x o x ==> x */
+ return LEFTFOLD;
+ return NEXTFOLD;
+}
+
LJFOLD(BXOR any any)
LJFOLDF(comm_bxor)
{
diff --git a/src/lj_vmmath.c b/src/lj_vmmath.c
index b231d3e8..08ccf467 100644
--- a/src/lj_vmmath.c
+++ b/src/lj_vmmath.c
@@ -50,8 +50,8 @@ double lj_vm_foldarith(double x, double y, int op)
#if LJ_HASJIT
case IR_ATAN2 - IR_ADD: return atan2(x, y); break;
case IR_LDEXP - IR_ADD: return ldexp(x, (int)y); break;
- case IR_MIN - IR_ADD: return x > y ? y : x; break;
- case IR_MAX - IR_ADD: return x < y ? y : x; break;
+ case IR_MIN - IR_ADD: return x < y ? x : y; break;
+ case IR_MAX - IR_ADD: return x > y ? x : y; break;
#endif
default: return x;
}
diff --git a/src/vm_arm.dasc b/src/vm_arm.dasc
index 21f7fecb..6b511347 100644
--- a/src/vm_arm.dasc
+++ b/src/vm_arm.dasc
@@ -1718,8 +1718,8 @@ static void build_subroutines(BuildCtx *ctx)
|.endif
|.endmacro
|
- | math_minmax math_min, gt, hi
- | math_minmax math_max, lt, lo
+ | math_minmax math_min, gt, pl
+ | math_minmax math_max, lt, le
|
|//-- String library -----------------------------------------------------
|
diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc
index 313cc94f..80e795ae 100644
--- a/src/vm_arm64.dasc
+++ b/src/vm_arm64.dasc
@@ -1491,8 +1491,8 @@ static void build_subroutines(BuildCtx *ctx)
| b <6
|.endmacro
|
- | math_minmax math_min, gt, hi
- | math_minmax math_max, lt, lo
+ | math_minmax math_min, gt, pl
+ | math_minmax math_max, lt, le
|
|//-- String library -----------------------------------------------------
|
diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
index 974047d3..cbf5fb9b 100644
--- a/src/vm_x64.dasc
+++ b/src/vm_x64.dasc
@@ -1874,7 +1874,7 @@ static void build_subroutines(BuildCtx *ctx)
| jmp ->fff_res
|
|.macro math_minmax, name, cmovop, sseop
- | .ffunc name
+ | .ffunc_1 name
| mov RAd, 2
|.if DUALNUM
| mov RB, [BASE]
diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
index ab8e6f27..b839eff0 100644
--- a/src/vm_x86.dasc
+++ b/src/vm_x86.dasc
@@ -2292,7 +2292,7 @@ static void build_subroutines(BuildCtx *ctx)
| xorps xmm4, xmm4; jmp <1 // Return +-Inf and +-0.
|
|.macro math_minmax, name, cmovop, sseop
- | .ffunc name
+ | .ffunc_1 name
| mov RA, 2
| cmp dword [BASE+4], LJ_TISNUM
|.if DUALNUM
diff --git a/test/tarantool-tests/gh-6163-jit-min-max.test.lua b/test/tarantool-tests/gh-6163-jit-min-max.test.lua
new file mode 100644
index 00000000..d6eb3f3b
--- /dev/null
+++ b/test/tarantool-tests/gh-6163-jit-min-max.test.lua
@@ -0,0 +1,20 @@
+local tap = require('tap')
+jit.off()
+jit.flush()
+
+local test = tap.test("gh-6163-jit-min-max")
+test:plan(2)
+--
+-- gh-6163: math.min/math.max success with no args
+--
+local pcall = pcall
+
+jit.opt.start(0, 'hotloop=1')
+jit.on()
+
+local r, _ = pcall(function() math.min() end)
+test:ok(false == r)
+r, _ = pcall(function() math.min() end)
+test:ok(false == r)
+
+os.exit(test:check() and 0 or 1)
--
2.35.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Tarantool-patches] [PATCH luajit v4 2/3] Don't compile math.modf() anymore.
2022-03-29 23:48 [Tarantool-patches] [PATCH luajit v4 0/3] fix math.min/max inconsistencies Maxim Kokryashkin via Tarantool-patches
2022-03-29 23:48 ` [Tarantool-patches] [PATCH luajit v4 1/3] Fix math.min()/math.max() inconsistencies Maxim Kokryashkin via Tarantool-patches
@ 2022-03-29 23:48 ` Maxim Kokryashkin via Tarantool-patches
2022-04-14 8:53 ` Sergey Kaplun via Tarantool-patches
2022-03-29 23:48 ` [Tarantool-patches] [PATCH luajit v4 3/3] Cleanup math function compilation and fix inconsistencies Maxim Kokryashkin via Tarantool-patches
2 siblings, 1 reply; 7+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2022-03-29 23:48 UTC (permalink / raw)
To: tarantool-patches, imun, skaplun
From: Mike Pall <mike>
This commit disables compilation for the `math.modf` function since it's
rarely used and properly compiling it would be difficult.
Part of tarantool/tarantool#6163
---
src/lib_math.c | 2 +-
src/lj_ffrecord.c | 16 -------------
.../gh-6163-jit-min-max.test.lua | 23 +++++++++++++++----
3 files changed, 20 insertions(+), 21 deletions(-)
diff --git a/src/lib_math.c b/src/lib_math.c
index ef9dda2d..4e6d2458 100644
--- a/src/lib_math.c
+++ b/src/lib_math.c
@@ -45,7 +45,7 @@ LJLIB_ASM_(math_sinh) LJLIB_REC(math_htrig IRCALL_sinh)
LJLIB_ASM_(math_cosh) LJLIB_REC(math_htrig IRCALL_cosh)
LJLIB_ASM_(math_tanh) LJLIB_REC(math_htrig IRCALL_tanh)
LJLIB_ASM_(math_frexp)
-LJLIB_ASM_(math_modf) LJLIB_REC(.)
+LJLIB_ASM_(math_modf)
LJLIB_ASM(math_log) LJLIB_REC(math_log)
{
diff --git a/src/lj_ffrecord.c b/src/lj_ffrecord.c
index be890a93..ac9c9ba1 100644
--- a/src/lj_ffrecord.c
+++ b/src/lj_ffrecord.c
@@ -601,22 +601,6 @@ static void LJ_FASTCALL recff_math_htrig(jit_State *J, RecordFFData *rd)
J->base[0] = emitir(IRTN(IR_CALLN), tr, rd->data);
}
-static void LJ_FASTCALL recff_math_modf(jit_State *J, RecordFFData *rd)
-{
- TRef tr = J->base[0];
- if (tref_isinteger(tr)) {
- J->base[0] = tr;
- J->base[1] = lj_ir_kint(J, 0);
- } else {
- TRef trt;
- tr = lj_ir_tonum(J, tr);
- trt = emitir(IRTN(IR_FPMATH), tr, IRFPM_TRUNC);
- J->base[0] = trt;
- J->base[1] = emitir(IRTN(IR_SUB), tr, trt);
- }
- rd->nres = 2;
-}
-
static void LJ_FASTCALL recff_math_pow(jit_State *J, RecordFFData *rd)
{
J->base[0] = lj_opt_narrow_pow(J, J->base[0], J->base[1],
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 d6eb3f3b..4594b968 100644
--- a/test/tarantool-tests/gh-6163-jit-min-max.test.lua
+++ b/test/tarantool-tests/gh-6163-jit-min-max.test.lua
@@ -1,12 +1,12 @@
local tap = require('tap')
+local jutil = require('jit.util')
jit.off()
jit.flush()
local test = tap.test("gh-6163-jit-min-max")
-test:plan(2)
---
--- gh-6163: math.min/math.max success with no args
---
+test:plan(6)
+
+-- `math.min`/`math.max` success with no args.
local pcall = pcall
jit.opt.start(0, 'hotloop=1')
@@ -17,4 +17,19 @@ test:ok(false == r)
r, _ = pcall(function() math.min() end)
test:ok(false == r)
+jit.off()
+jit.flush()
+jit.on()
+
+-- `math.modf` shouldn't be compiled.
+for _ = 1, 3 do math.modf(5.32) end
+
+local tr1_info = jutil.traceinfo(1)
+local tr2_info = jutil.traceinfo(2)
+
+test:ok(tr1_info ~= nil)
+test:ok(tr2_info ~= nil)
+test:ok(tr1_info.link == 2)
+test:ok(tr1_info.linktype == "stitch")
+
os.exit(test:check() and 0 or 1)
--
2.35.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Tarantool-patches] [PATCH luajit v4 3/3] Cleanup math function compilation and fix inconsistencies.
2022-03-29 23:48 [Tarantool-patches] [PATCH luajit v4 0/3] fix math.min/max inconsistencies Maxim Kokryashkin via Tarantool-patches
2022-03-29 23:48 ` [Tarantool-patches] [PATCH luajit v4 1/3] Fix math.min()/math.max() inconsistencies Maxim Kokryashkin via Tarantool-patches
2022-03-29 23:48 ` [Tarantool-patches] [PATCH luajit v4 2/3] Don't compile math.modf() anymore Maxim Kokryashkin via Tarantool-patches
@ 2022-03-29 23:48 ` Maxim Kokryashkin via Tarantool-patches
2022-04-14 8:53 ` Sergey Kaplun via Tarantool-patches
2 siblings, 1 reply; 7+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2022-03-29 23:48 UTC (permalink / raw)
To: tarantool-patches, imun, skaplun
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`).
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
@@ -33,17 +33,17 @@ LJLIB_ASM(math_sqrt) LJLIB_REC(math_unary IRFPM_SQRT)
lj_lib_checknum(L, 1);
return FFH_RETRY;
}
-LJLIB_ASM_(math_log10) LJLIB_REC(math_unary IRFPM_LOG10)
-LJLIB_ASM_(math_exp) LJLIB_REC(math_unary IRFPM_EXP)
-LJLIB_ASM_(math_sin) LJLIB_REC(math_unary IRFPM_SIN)
-LJLIB_ASM_(math_cos) LJLIB_REC(math_unary IRFPM_COS)
-LJLIB_ASM_(math_tan) LJLIB_REC(math_unary IRFPM_TAN)
-LJLIB_ASM_(math_asin) LJLIB_REC(math_atrig FF_math_asin)
-LJLIB_ASM_(math_acos) LJLIB_REC(math_atrig FF_math_acos)
-LJLIB_ASM_(math_atan) LJLIB_REC(math_atrig FF_math_atan)
-LJLIB_ASM_(math_sinh) LJLIB_REC(math_htrig IRCALL_sinh)
-LJLIB_ASM_(math_cosh) LJLIB_REC(math_htrig IRCALL_cosh)
-LJLIB_ASM_(math_tanh) LJLIB_REC(math_htrig IRCALL_tanh)
+LJLIB_ASM_(math_log10) LJLIB_REC(math_call IRCALL_log10)
+LJLIB_ASM_(math_exp) LJLIB_REC(math_call IRCALL_exp)
+LJLIB_ASM_(math_sin) LJLIB_REC(math_call IRCALL_sin)
+LJLIB_ASM_(math_cos) LJLIB_REC(math_call IRCALL_cos)
+LJLIB_ASM_(math_tan) LJLIB_REC(math_call IRCALL_tan)
+LJLIB_ASM_(math_asin) LJLIB_REC(math_call IRCALL_asin)
+LJLIB_ASM_(math_acos) LJLIB_REC(math_call IRCALL_acos)
+LJLIB_ASM_(math_atan) LJLIB_REC(math_call IRCALL_atan)
+LJLIB_ASM_(math_sinh) LJLIB_REC(math_call IRCALL_sinh)
+LJLIB_ASM_(math_cosh) LJLIB_REC(math_call IRCALL_cosh)
+LJLIB_ASM_(math_tanh) LJLIB_REC(math_call IRCALL_tanh)
LJLIB_ASM_(math_frexp)
LJLIB_ASM_(math_modf)
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
@@ -1660,7 +1660,6 @@ static void asm_ir(ASMState *as, IRIns *ir)
case IR_DIV: asm_div(as, ir); break;
case IR_POW: asm_pow(as, ir); break;
case IR_ABS: asm_abs(as, ir); break;
- case IR_ATAN2: asm_atan2(as, ir); break;
case IR_LDEXP: asm_ldexp(as, ir); break;
case IR_FPMATH: asm_fpmath(as, ir); break;
case IR_TOBIT: asm_tobit(as, ir); break;
@@ -2150,11 +2149,6 @@ static void asm_setup_regsp(ASMState *as)
as->modset = RSET_SCRATCH;
break;
#if !LJ_SOFTFP
- case IR_ATAN2:
-#if LJ_TARGET_X86
- if (as->evenspill < 4) /* Leave room to call atan2(). */
- as->evenspill = 4;
-#endif
#if !LJ_TARGET_X86ORX64
case IR_LDEXP:
#endif
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
@@ -1502,7 +1502,6 @@ static void asm_mul(ASMState *as, IRIns *ir)
#define asm_div(as, ir) asm_fparith(as, ir, ARMI_VDIV_D)
#define asm_pow(as, ir) asm_callid(as, ir, IRCALL_lj_vm_powi)
#define asm_abs(as, ir) asm_fpunary(as, ir, ARMI_VABS_D)
-#define asm_atan2(as, ir) asm_callid(as, ir, IRCALL_atan2)
#define asm_ldexp(as, ir) asm_callid(as, ir, IRCALL_ldexp)
#endif
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
@@ -1451,7 +1451,6 @@ static void asm_pow(ASMState *as, IRIns *ir)
#define asm_mulov(as, ir) asm_mul(as, ir)
#define asm_abs(as, ir) asm_fpunary(as, ir, A64I_FABS)
-#define asm_atan2(as, ir) asm_callid(as, ir, IRCALL_atan2)
#define asm_ldexp(as, ir) asm_callid(as, ir, IRCALL_ldexp)
static void asm_mod(ASMState *as, IRIns *ir)
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
@@ -1970,8 +1970,6 @@ static void asm_fpmath(ASMState *as, IRIns *ir)
}
}
-#define asm_atan2(as, ir) asm_callid(as, ir, IRCALL_atan2)
-
static void asm_ldexp(ASMState *as, IRIns *ir)
{
int32_t ofs = sps_scale(ir->s); /* Use spill slot or temp slots. */
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
@@ -563,7 +563,7 @@ static void LJ_FASTCALL recff_math_atan2(jit_State *J, RecordFFData *rd)
{
TRef tr = lj_ir_tonum(J, J->base[0]);
TRef tr2 = lj_ir_tonum(J, J->base[1]);
- J->base[0] = emitir(IRTN(IR_ATAN2), tr, tr2);
+ J->base[0] = lj_ir_call(J, IRCALL_atan2, tr, tr2);
UNUSED(rd);
}
@@ -580,22 +580,7 @@ static void LJ_FASTCALL recff_math_ldexp(jit_State *J, RecordFFData *rd)
UNUSED(rd);
}
-/* Record math.asin, math.acos, math.atan. */
-static void LJ_FASTCALL recff_math_atrig(jit_State *J, RecordFFData *rd)
-{
- TRef y = lj_ir_tonum(J, J->base[0]);
- TRef x = lj_ir_knum_one(J);
- uint32_t ffid = rd->data;
- if (ffid != FF_math_atan) {
- TRef tmp = emitir(IRTN(IR_MUL), y, y);
- tmp = emitir(IRTN(IR_SUB), x, tmp);
- tmp = emitir(IRTN(IR_FPMATH), tmp, IRFPM_SQRT);
- if (ffid == FF_math_asin) { x = tmp; } else { x = y; y = tmp; }
- }
- J->base[0] = emitir(IRTN(IR_ATAN2), y, x);
-}
-
-static void LJ_FASTCALL recff_math_htrig(jit_State *J, RecordFFData *rd)
+static void LJ_FASTCALL recff_math_call(jit_State *J, RecordFFData *rd)
{
TRef tr = lj_ir_tonum(J, J->base[0]);
J->base[0] = emitir(IRTN(IR_CALLN), tr, rd->data);
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
@@ -75,7 +75,6 @@
_(NEG, N , ref, ref) \
\
_(ABS, N , ref, ref) \
- _(ATAN2, N , ref, ref) \
_(LDEXP, N , ref, ref) \
_(MIN, C , ref, ref) \
_(MAX, C , ref, ref) \
@@ -178,8 +177,7 @@ LJ_STATIC_ASSERT((int)IR_XLOAD + IRDELTA_L2S == (int)IR_XSTORE);
/* FPMATH sub-functions. ORDER FPM. */
#define IRFPMDEF(_) \
_(FLOOR) _(CEIL) _(TRUNC) /* Must be first and in this order. */ \
- _(SQRT) _(EXP) _(EXP2) _(LOG) _(LOG2) _(LOG10) \
- _(SIN) _(COS) _(TAN) \
+ _(SQRT) _(EXP2) _(LOG) _(LOG2) \
_(OTHER)
typedef enum {
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
@@ -21,6 +21,7 @@ typedef struct CCallInfo {
#define CCI_OTSHIFT 16
#define CCI_OPTYPE(ci) ((ci)->flags >> CCI_OTSHIFT) /* Get op/type. */
+#define CCI_TYPE(ci) (((ci)->flags>>CCI_OTSHIFT) & IRT_TYPE)
#define CCI_OPSHIFT 24
#define CCI_OP(ci) ((ci)->flags >> CCI_OPSHIFT) /* Get op. */
@@ -158,6 +159,14 @@ typedef struct CCallInfo {
_(ANY, lj_mem_newgco, 2, FS, PGC, CCI_L) \
_(ANY, lj_math_random_step, 1, FS, NUM, CCI_CASTU64) \
_(ANY, lj_vm_modi, 2, FN, INT, 0) \
+ _(ANY, log10, 1, N, NUM, XA_FP) \
+ _(ANY, exp, 1, N, NUM, XA_FP) \
+ _(ANY, sin, 1, N, NUM, XA_FP) \
+ _(ANY, cos, 1, N, NUM, XA_FP) \
+ _(ANY, tan, 1, N, NUM, XA_FP) \
+ _(ANY, asin, 1, N, NUM, XA_FP) \
+ _(ANY, acos, 1, N, NUM, XA_FP) \
+ _(ANY, atan, 1, N, NUM, XA_FP) \
_(ANY, sinh, 1, N, NUM, XA_FP) \
_(ANY, cosh, 1, N, NUM, XA_FP) \
_(ANY, tanh, 1, N, NUM, XA_FP) \
@@ -169,14 +178,9 @@ typedef struct CCallInfo {
_(FPMATH, lj_vm_ceil, 1, N, NUM, XA_FP) \
_(FPMATH, lj_vm_trunc, 1, N, NUM, XA_FP) \
_(FPMATH, sqrt, 1, N, NUM, XA_FP) \
- _(ANY, exp, 1, N, NUM, XA_FP) \
_(ANY, lj_vm_exp2, 1, N, NUM, XA_FP) \
_(ANY, log, 1, N, NUM, XA_FP) \
_(ANY, lj_vm_log2, 1, N, NUM, XA_FP) \
- _(ANY, log10, 1, N, NUM, XA_FP) \
- _(ANY, sin, 1, N, NUM, XA_FP) \
- _(ANY, cos, 1, N, NUM, XA_FP) \
- _(ANY, tan, 1, N, NUM, XA_FP) \
_(ANY, lj_vm_powi, 2, N, NUM, XA_FP) \
_(ANY, pow, 2, N, NUM, XA2_FP) \
_(ANY, atan2, 2, N, NUM, XA2_FP) \
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
@@ -426,9 +426,6 @@ static void split_ir(jit_State *J)
}
hi = split_call_l(J, hisubst, oir, ir, IRCALL_lj_vm_floor + ir->op2);
break;
- case IR_ATAN2:
- hi = split_call_ll(J, hisubst, oir, ir, IRCALL_atan2);
- break;
case IR_LDEXP:
hi = split_call_li(J, hisubst, oir, ir, IRCALL_ldexp);
break;
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 {
/* Note: little-endian byte-order! */
XI_FLDZ = 0xeed9,
XI_FLD1 = 0xe8d9,
- XI_FLDLG2 = 0xecd9,
- XI_FLDLN2 = 0xedd9,
XI_FDUP = 0xc0d9, /* Really fld st0. */
XI_FPOP = 0xd8dd, /* Really fstp st0. */
XI_FPOP1 = 0xd9dd, /* Really fstp st1. */
XI_FRNDINT = 0xfcd9,
- XI_FSIN = 0xfed9,
- XI_FCOS = 0xffd9,
- XI_FPTAN = 0xf2d9,
- XI_FPATAN = 0xf3d9,
XI_FSCALE = 0xfdd9,
XI_FYL2X = 0xf1d9,
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
@@ -48,7 +48,6 @@ double lj_vm_foldarith(double x, double y, int op)
case IR_NEG - IR_ADD: return -x; break;
case IR_ABS - IR_ADD: return fabs(x); break;
#if LJ_HASJIT
- case IR_ATAN2 - IR_ADD: return atan2(x, y); break;
case IR_LDEXP - IR_ADD: return ldexp(x, (int)y); break;
case IR_MIN - IR_ADD: return x < y ? x : y; break;
case IR_MAX - IR_ADD: return x > y ? x : y; break;
@@ -129,14 +128,9 @@ double lj_vm_foldfpm(double x, int fpm)
case IRFPM_CEIL: return lj_vm_ceil(x);
case IRFPM_TRUNC: return lj_vm_trunc(x);
case IRFPM_SQRT: return sqrt(x);
- case IRFPM_EXP: return exp(x);
case IRFPM_EXP2: return lj_vm_exp2(x);
case IRFPM_LOG: return log(x);
case IRFPM_LOG2: return lj_vm_log2(x);
- case IRFPM_LOG10: return log10(x);
- case IRFPM_SIN: return sin(x);
- case IRFPM_COS: return cos(x);
- case IRFPM_TAN: return tan(x);
default: lua_assert(0);
}
return 0;
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 @@
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
+
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit v4 3/3] Cleanup math function compilation and fix inconsistencies.
2022-03-29 23:48 ` [Tarantool-patches] [PATCH luajit v4 3/3] Cleanup math function compilation and fix inconsistencies Maxim Kokryashkin via Tarantool-patches
@ 2022-04-14 8:53 ` Sergey Kaplun via Tarantool-patches
0 siblings, 0 replies; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2022-04-14 8:53 UTC (permalink / raw)
To: Maxim Kokryashkin; +Cc: tarantool-patches
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit v4 2/3] Don't compile math.modf() anymore.
2022-03-29 23:48 ` [Tarantool-patches] [PATCH luajit v4 2/3] Don't compile math.modf() anymore Maxim Kokryashkin via Tarantool-patches
@ 2022-04-14 8:53 ` Sergey Kaplun via Tarantool-patches
0 siblings, 0 replies; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2022-04-14 8:53 UTC (permalink / raw)
To: Maxim Kokryashkin; +Cc: tarantool-patches
Hi, Maxim!
Thanks for the patch.
It generally LGTM, except the following concerns:
1) The tests kinda useless since they check that `modf()` just not compiled
instead checking incorrect behaviour of `modf()` -- the reason for
disabling compilation.
2) These tests are not related to min/max functions, so they should be
moved to the separate test file.
On 30.03.22, Maxim Kokryashkin wrote:
> From: Mike Pall <mike>
>
> This commit disables compilation for the `math.modf` function since it's
> rarely used and properly compiling it would be difficult.
>
> Part of tarantool/tarantool#6163
> ---
> src/lib_math.c | 2 +-
> src/lj_ffrecord.c | 16 -------------
> .../gh-6163-jit-min-max.test.lua | 23 +++++++++++++++----
> 3 files changed, 20 insertions(+), 21 deletions(-)
>
> diff --git a/src/lib_math.c b/src/lib_math.c
> index ef9dda2d..4e6d2458 100644
> --- a/src/lib_math.c
> +++ b/src/lib_math.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 d6eb3f3b..4594b968 100644
> --- a/test/tarantool-tests/gh-6163-jit-min-max.test.lua
> +++ b/test/tarantool-tests/gh-6163-jit-min-max.test.lua
> @@ -1,12 +1,12 @@
> local tap = require('tap')
> +local jutil = require('jit.util')
> jit.off()
> jit.flush()
>
> local test = tap.test("gh-6163-jit-min-max")
> -test:plan(2)
> ---
> --- gh-6163: math.min/math.max success with no args
> ---
> +test:plan(6)
> +
> +-- `math.min`/`math.max` success with no args.
> local pcall = pcall
>
> jit.opt.start(0, 'hotloop=1')
> @@ -17,4 +17,19 @@ test:ok(false == r)
> r, _ = pcall(function() math.min() end)
> test:ok(false == r)
>
> +jit.off()
> +jit.flush()
> +jit.on()
> +
> +-- `math.modf` shouldn't be compiled.
> +for _ = 1, 3 do math.modf(5.32) end
> +
> +local tr1_info = jutil.traceinfo(1)
> +local tr2_info = jutil.traceinfo(2)
> +
> +test:ok(tr1_info ~= nil)
> +test:ok(tr2_info ~= nil)
> +test:ok(tr1_info.link == 2)
> +test:ok(tr1_info.linktype == "stitch")
> +
> os.exit(test:check() and 0 or 1)
> --
> 2.35.1
>
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit v4 1/3] Fix math.min()/math.max() inconsistencies.
2022-03-29 23:48 ` [Tarantool-patches] [PATCH luajit v4 1/3] Fix math.min()/math.max() inconsistencies Maxim Kokryashkin via Tarantool-patches
@ 2022-04-14 8:55 ` Sergey Kaplun via Tarantool-patches
0 siblings, 0 replies; 7+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2022-04-14 8:55 UTC (permalink / raw)
To: Maxim Kokryashkin; +Cc: tarantool-patches
Hi, Maxim!
Thanks for the patch!
Please, consider my comments below.
On 30.03.22, Maxim Kokryashkin wrote:
> From: Mike Pall <mike>
>
> math.min()/math.max() could produce different results when compiled.
> Previously, these functions did not check for the number of arguments,
Nit: Didn't check number of arguments for x86/x64 architecture.
> which in some situations led to incorrect behavior.
Nit: I suppose "in some situatitons" may be omitted (these situations is
incorrect (i.e. 0 -- we can mention that) amount of arguments).
>
> Consider this example:
> ```
> jit.opt.start(0, "hotloop=1")
>
> local r, msg = pcall(function () math.min() end)
> r, msg = pcall(function () math.min() end)
> ```
> Before this patch, this snippet of code completed successfully, although it
> was not supposed to, since there were no args to math.min().
Nit: I suggest to drop this snippet as far as you allready describe the
behaviour in such case.
>
> This patch adds check for the number of arguments for jit-compiled
Does it?
AFAICS, it only adds the corresponding check in the VM (ffunc->ffunc_1), so
there is no incorrect recording as far as we always fall through to ff
fallback.
> version of math.min/max, and it also adds the corresponding test case for
> the mentioned issue.
We should mention about refactoring, too. I see the following parts:
* removing several associative fold optimizations (we need to check
logic of this optimization, and add the corresponding testcases)
* changing fcc in asm_min_max CC_HI/CC_LO -> CC_PL/CC_PL (maybe you
may investigate this and create a testcase showing "inconsistencies")
>
> Part of tarantool/tarantool#6163
I tried the reproducer mentioned in this ticket [1] and its passes with
only this patch. So patches in this patchset may be merged separated
(but in the same order).
> Part of tarantool/tarantool#6548
> ---
> src/lj_asm_arm.h | 6 +--
> src/lj_asm_arm64.h | 6 +--
> src/lj_opt_fold.c | 53 +++++++------------
> src/lj_vmmath.c | 4 +-
> src/vm_arm.dasc | 4 +-
> src/vm_arm64.dasc | 4 +-
> src/vm_x64.dasc | 2 +-
> src/vm_x86.dasc | 2 +-
> .../gh-6163-jit-min-max.test.lua | 20 +++++++
> 9 files changed, 53 insertions(+), 48 deletions(-)
> create mode 100644 test/tarantool-tests/gh-6163-jit-min-max.test.lua
>
> diff --git 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
<snipped>
> diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
<snipped>
> diff --git a/src/vm_arm.dasc b/src/vm_arm.dasc
<snipped>
> diff --git a/src/vm_arm64.dasc b/src/vm_arm64.dasc
<snipped>
> diff --git a/src/vm_x64.dasc b/src/vm_x64.dasc
<snipped>
> diff --git a/src/vm_x86.dasc b/src/vm_x86.dasc
<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
> new file mode 100644
> index 00000000..d6eb3f3b
> --- /dev/null
> +++ b/test/tarantool-tests/gh-6163-jit-min-max.test.lua
> @@ -0,0 +1,20 @@
I've checked your test on my branch without your patchset and it still
passes:
| $ ../../src/luajit gh-6163-jit-min-max.test.lua
| TAP version 13
| 1..2
| ok - nil
| ok - nil
| $ make tarantool-tests
|
| /home/burii/builds_workspace/luajit/fix-slot-check-for-mm-record/test/tarantool-tests/gh-6163-jit-min-max.test.lua ......................... ok
> +local tap = require('tap')
> +jit.off()
> +jit.flush()
> +
> +local test = tap.test("gh-6163-jit-min-max")
Nit: Please, use one type of quotes: ''.
> +test:plan(2)
> +--
> +-- gh-6163: math.min/math.max success with no args
> +--
> +local pcall = pcall
> +
> +jit.opt.start(0, 'hotloop=1')
> +jit.on()
> +
> +local r, _ = pcall(function() math.min() end)
> +test:ok(false == r)
> +r, _ = pcall(function() math.min() end)
> +test:ok(false == r)
> +
> +os.exit(test:check() and 0 or 1)
[1]: https://github.com/tarantool/tarantool/issues/6163
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-04-14 8:57 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-29 23:48 [Tarantool-patches] [PATCH luajit v4 0/3] fix math.min/max inconsistencies Maxim Kokryashkin via Tarantool-patches
2022-03-29 23:48 ` [Tarantool-patches] [PATCH luajit v4 1/3] Fix math.min()/math.max() inconsistencies Maxim Kokryashkin via Tarantool-patches
2022-04-14 8:55 ` Sergey Kaplun via Tarantool-patches
2022-03-29 23:48 ` [Tarantool-patches] [PATCH luajit v4 2/3] Don't compile math.modf() anymore Maxim Kokryashkin via Tarantool-patches
2022-04-14 8:53 ` Sergey Kaplun via Tarantool-patches
2022-03-29 23:48 ` [Tarantool-patches] [PATCH luajit v4 3/3] Cleanup math function compilation and fix inconsistencies Maxim Kokryashkin via Tarantool-patches
2022-04-14 8:53 ` Sergey Kaplun via Tarantool-patches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox