* [Tarantool-patches] [PATCH luajit 0/2] Fixes for 64 bit operands of the bit library
@ 2024-10-02 8:09 Sergey Kaplun via Tarantool-patches
2024-10-02 8:09 ` [Tarantool-patches] [PATCH luajit 1/2] Fix bit op coercion in DUALNUM builds Sergey Kaplun via Tarantool-patches
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-10-02 8:09 UTC (permalink / raw)
To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches
This patchset fixes a bunch of misbehaviours for the 64-bit-wide
operands in the built-in `bit` library.
Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1079-fix-64-bitshift-folds
Related issues:
* https://github.com/tarantool/tarantool/issues/10199
* https://github.com/LuaJIT/LuaJIT/issues/1079
* https://github.com/LuaJIT/LuaJIT/issues/1273
Mike Pall (2):
Fix bit op coercion in DUALNUM builds.
FFI: Fix 64 bit shift fold rules.
src/lj_carith.c | 4 +-
src/lj_opt_fold.c | 8 +-
.../lj-1079-fix-64-bitshift-folds.test.lua | 74 +++++++++++++++++++
.../lj-1273-dualnum-bit-coercion.test.lua | 19 +++++
4 files changed, 98 insertions(+), 7 deletions(-)
create mode 100644 test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
create mode 100644 test/tarantool-tests/lj-1273-dualnum-bit-coercion.test.lua
--
2.46.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Tarantool-patches] [PATCH luajit 1/2] Fix bit op coercion in DUALNUM builds.
2024-10-02 8:09 [Tarantool-patches] [PATCH luajit 0/2] Fixes for 64 bit operands of the bit library Sergey Kaplun via Tarantool-patches
@ 2024-10-02 8:09 ` Sergey Kaplun via Tarantool-patches
2024-10-08 10:12 ` Sergey Bronnikov via Tarantool-patches
2024-10-11 19:08 ` Maxim Kokryashkin via Tarantool-patches
2024-10-02 8:09 ` [Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules Sergey Kaplun via Tarantool-patches
2024-10-18 15:17 ` [Tarantool-patches] [PATCH luajit 0/2] Fixes for 64 bit operands of the bit library Sergey Kaplun via Tarantool-patches
2 siblings, 2 replies; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-10-02 8:09 UTC (permalink / raw)
To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches
From: Mike Pall <mike>
Thanks to Sergey Kaplun.
(cherry picked from commit f5fd22203eadf57ccbaa4a298010d23974b22fc0)
The `lj_carith_check64()` function coerces the given number value to the
32-bit wide value. In this case, the 64-bit-wide operands will lose
upper bits.
This patch removes the excess coercion for the DUALNUM mode.
Sergey Kaplun:
* added the description and the test for the problem
Part of tarantool/tarantool#10199
---
src/lj_carith.c | 4 +---
.../lj-1273-dualnum-bit-coercion.test.lua | 19 +++++++++++++++++++
2 files changed, 20 insertions(+), 3 deletions(-)
create mode 100644 test/tarantool-tests/lj-1273-dualnum-bit-coercion.test.lua
diff --git a/src/lj_carith.c b/src/lj_carith.c
index 90b3220f..eb56d552 100644
--- a/src/lj_carith.c
+++ b/src/lj_carith.c
@@ -349,9 +349,7 @@ uint64_t lj_carith_check64(lua_State *L, int narg, CTypeID *id)
if (LJ_LIKELY(tvisint(o))) {
return (uint32_t)intV(o);
} else {
- int32_t i = lj_num2bit(numV(o));
- if (LJ_DUALNUM) setintV(o, i);
- return (uint32_t)i;
+ return (uint32_t)lj_num2bit(numV(o));
}
}
diff --git a/test/tarantool-tests/lj-1273-dualnum-bit-coercion.test.lua b/test/tarantool-tests/lj-1273-dualnum-bit-coercion.test.lua
new file mode 100644
index 00000000..e83dbbcd
--- /dev/null
+++ b/test/tarantool-tests/lj-1273-dualnum-bit-coercion.test.lua
@@ -0,0 +1,19 @@
+local tap = require('tap')
+
+-- Test file to demonstrate LuaJIT misbehaviour on operating
+-- for 64-bit operands in built-in `bit` library in DUALNUM mode.
+-- See also, https://github.com/LuaJIT/LuaJIT/issues/1273.
+
+local test = tap.test('lj-1273-dualnum-bit-coercion')
+test:plan(1)
+
+local bit = require('bit')
+
+-- The cdata value for 2 ^ 33.
+local EXPECTED = 2 ^ 33 + 0LL
+-- Same value is used as mask for `bit.band()`.
+local MASK = EXPECTED
+
+test:is(bit.band(2 ^ 33, MASK), EXPECTED, 'correct bit.band result')
+
+test:done(true)
--
2.46.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* [Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules.
2024-10-02 8:09 [Tarantool-patches] [PATCH luajit 0/2] Fixes for 64 bit operands of the bit library Sergey Kaplun via Tarantool-patches
2024-10-02 8:09 ` [Tarantool-patches] [PATCH luajit 1/2] Fix bit op coercion in DUALNUM builds Sergey Kaplun via Tarantool-patches
@ 2024-10-02 8:09 ` Sergey Kaplun via Tarantool-patches
2024-10-08 12:07 ` Sergey Bronnikov via Tarantool-patches
2024-10-18 15:17 ` [Tarantool-patches] [PATCH luajit 0/2] Fixes for 64 bit operands of the bit library Sergey Kaplun via Tarantool-patches
2 siblings, 1 reply; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-10-02 8:09 UTC (permalink / raw)
To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches
From: Mike Pall <mike>
Thanks to Peter Cawley.
(cherry picked from commit 9e0437240f1fb4bfa7248f6ec8be0e3181016119)
For `IR_BSHR`, `IR_BROL`, `IR_BROR` during `kfold_int64arith()` the left
argument is truncated down to 32 bits, which leads to incorrect results
if the right argument is >= 32.
Also, `IR_BSAR` does an unsigned shift rather than a signed shift, but
since this case branch is unreachable, it is harmless for now.
This patch fixes all misbehaviours (including possible for `IR_BSAR`) to
preserve IR semantics.
Sergey Kaplun:
* added the description and the test for the problem
Part of tarantool/tarantool#10199
---
src/lj_opt_fold.c | 8 +-
.../lj-1079-fix-64-bitshift-folds.test.lua | 74 +++++++++++++++++++
2 files changed, 78 insertions(+), 4 deletions(-)
create mode 100644 test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
index e2171e1b..2702f79f 100644
--- a/src/lj_opt_fold.c
+++ b/src/lj_opt_fold.c
@@ -382,10 +382,10 @@ static uint64_t kfold_int64arith(jit_State *J, uint64_t k1, uint64_t k2,
case IR_BOR: k1 |= k2; break;
case IR_BXOR: k1 ^= k2; break;
case IR_BSHL: k1 <<= (k2 & 63); break;
- case IR_BSHR: k1 = (int32_t)((uint32_t)k1 >> (k2 & 63)); break;
- case IR_BSAR: k1 >>= (k2 & 63); break;
- case IR_BROL: k1 = (int32_t)lj_rol((uint32_t)k1, (k2 & 63)); break;
- case IR_BROR: k1 = (int32_t)lj_ror((uint32_t)k1, (k2 & 63)); break;
+ case IR_BSHR: k1 >>= (k2 & 63); break;
+ case IR_BSAR: k1 = (uint64_t)((int64_t)k1 >> (k2 & 63)); break;
+ case IR_BROL: k1 = lj_rol(k1, (k2 & 63)); break;
+ case IR_BROR: k1 = lj_ror(k1, (k2 & 63)); break;
default: lj_assertJ(0, "bad IR op %d", op); break;
}
#else
diff --git a/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua b/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
new file mode 100644
index 00000000..6cc0b319
--- /dev/null
+++ b/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
@@ -0,0 +1,74 @@
+local tap = require('tap')
+
+-- Test file to demonstrate LuaJIT misbehaviour on folding
+-- for bitshift operations.
+-- See also, https://github.com/LuaJIT/LuaJIT/issues/1079.
+
+local test = tap.test('lj-1079-fix-64-bitshift-folds'):skipcond({
+ ['Test requires JIT enabled'] = not jit.status(),
+})
+
+local bit = require('bit')
+
+test:plan(4)
+
+-- Generic function for `bit.ror()`, `bit.rol()`.
+local function bitop_rotation(bitop)
+ local r = {}
+ for i = 1, 4 do
+ -- (i & k1) o k2 ==> (i o k2) & (k1 o k2)
+ local int64 = bit.band(i, 7LL)
+ r[i] = tonumber(bitop(int64, 32))
+ end
+ return r
+end
+
+-- Similar function for `bit.rshift()`.
+local function bitop_rshift_signed()
+ local r = {}
+ for i = 1, 4 do
+ -- (i & k1) o k2 ==> (i o k2) & (k1 o k2)
+ -- XXX: Use `-i` instead of `i` to prevent other folding due
+ -- to IR difference so the IRs don't match fold rule mask.
+ -- (-i & 7LL) < 1 << 32 => result == 0.
+ local int64 = bit.band(-i, 7LL)
+ r[i] = tonumber(bit.rshift(int64, 32))
+ end
+ return r
+end
+
+-- A little bit different example, which leads to the assertion
+-- failure due to the incorrect recording.
+local function bitop_rshift_huge()
+ local r = {}
+ for i = 1, 4 do
+ -- (i & k1) o k2 ==> (i o k2) & (k1 o k2)
+ -- XXX: Need to use cast to the int64_t via `+ 0LL`, see the
+ -- documentation [1] for the details.
+ -- [1]: https://bitop.luajit.org/semantics.html
+ local int64 = bit.band(2 ^ 33 + i, 2 ^ 33 + 0LL)
+ r[i] = tonumber(bit.rshift(int64, 32))
+ end
+ return r
+end
+
+local function test_64bitness(subtest, payload_func, bitop)
+ subtest:plan(1)
+
+ jit.off()
+ jit.flush()
+ local results_joff = payload_func(bitop)
+ jit.on()
+ -- Reset hotcounters.
+ jit.opt.start('hotloop=1')
+ local results_jon = payload_func(bitop)
+ subtest:is_deeply(results_jon, results_joff,
+ 'same results for VM and JIT for ' .. subtest.name)
+end
+
+test:test('rol', test_64bitness, bitop_rotation, bit.rol)
+test:test('ror', test_64bitness, bitop_rotation, bit.ror)
+test:test('rshift signed', test_64bitness, bitop_rshift_signed)
+test:test('rshift huge', test_64bitness, bitop_rshift_huge)
+
+test:done(true)
--
2.46.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 1/2] Fix bit op coercion in DUALNUM builds.
2024-10-02 8:09 ` [Tarantool-patches] [PATCH luajit 1/2] Fix bit op coercion in DUALNUM builds Sergey Kaplun via Tarantool-patches
@ 2024-10-08 10:12 ` Sergey Bronnikov via Tarantool-patches
2024-10-11 19:08 ` Maxim Kokryashkin via Tarantool-patches
1 sibling, 0 replies; 10+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-10-08 10:12 UTC (permalink / raw)
To: Sergey Kaplun, Maxim Kokryashkin; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]
Hello, Sergey,
thanks for the patch! LGTM
On 02.10.2024 11:09, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Sergey Kaplun.
>
> (cherry picked from commit f5fd22203eadf57ccbaa4a298010d23974b22fc0)
>
> The `lj_carith_check64()` function coerces the given number value to the
> 32-bit wide value. In this case, the 64-bit-wide operands will lose
> upper bits.
>
> This patch removes the excess coercion for the DUALNUM mode.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#10199
> ---
> src/lj_carith.c | 4 +---
> .../lj-1273-dualnum-bit-coercion.test.lua | 19 +++++++++++++++++++
> 2 files changed, 20 insertions(+), 3 deletions(-)
> create mode 100644 test/tarantool-tests/lj-1273-dualnum-bit-coercion.test.lua
>
> diff --git a/src/lj_carith.c b/src/lj_carith.c
> index 90b3220f..eb56d552 100644
> --- a/src/lj_carith.c
> +++ b/src/lj_carith.c
> @@ -349,9 +349,7 @@ uint64_t lj_carith_check64(lua_State *L, int narg, CTypeID *id)
> if (LJ_LIKELY(tvisint(o))) {
> return (uint32_t)intV(o);
> } else {
> - int32_t i = lj_num2bit(numV(o));
> - if (LJ_DUALNUM) setintV(o, i);
> - return (uint32_t)i;
> + return (uint32_t)lj_num2bit(numV(o));
> }
> }
>
> diff --git a/test/tarantool-tests/lj-1273-dualnum-bit-coercion.test.lua b/test/tarantool-tests/lj-1273-dualnum-bit-coercion.test.lua
> new file mode 100644
> index 00000000..e83dbbcd
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1273-dualnum-bit-coercion.test.lua
> @@ -0,0 +1,19 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT misbehaviour on operating
> +-- for 64-bit operands in built-in `bit` library in DUALNUM mode.
> +-- See also,https://github.com/LuaJIT/LuaJIT/issues/1273.
> +
> +local test = tap.test('lj-1273-dualnum-bit-coercion')
> +test:plan(1)
> +
> +local bit = require('bit')
> +
> +-- The cdata value for 2 ^ 33.
> +local EXPECTED = 2 ^ 33 + 0LL
> +-- Same value is used as mask for `bit.band()`.
> +local MASK = EXPECTED
> +
> +test:is(bit.band(2 ^ 33, MASK), EXPECTED, 'correct bit.band result')
> +
> +test:done(true)
[-- Attachment #2: Type: text/html, Size: 2635 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules.
2024-10-02 8:09 ` [Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules Sergey Kaplun via Tarantool-patches
@ 2024-10-08 12:07 ` Sergey Bronnikov via Tarantool-patches
2024-10-08 14:24 ` Sergey Kaplun via Tarantool-patches
0 siblings, 1 reply; 10+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-10-08 12:07 UTC (permalink / raw)
To: Sergey Kaplun, Maxim Kokryashkin; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 4854 bytes --]
Hello, Sergey,
thanks for the patch! Please see my comments below.
On 02.10.2024 11:09, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Thanks to Peter Cawley.
>
> (cherry picked from commit 9e0437240f1fb4bfa7248f6ec8be0e3181016119)
>
> For `IR_BSHR`, `IR_BROL`, `IR_BROR` during `kfold_int64arith()` the left
> argument is truncated down to 32 bits, which leads to incorrect results
> if the right argument is >= 32.
typo: is >= 2,147,483,647
>
> Also, `IR_BSAR` does an unsigned shift rather than a signed shift, but
> since this case branch is unreachable, it is harmless for now.
>
> This patch fixes all misbehaviours (including possible for `IR_BSAR`) to
> preserve IR semantics.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> Part of tarantool/tarantool#10199
> ---
> src/lj_opt_fold.c | 8 +-
> .../lj-1079-fix-64-bitshift-folds.test.lua | 74 +++++++++++++++++++
> 2 files changed, 78 insertions(+), 4 deletions(-)
> create mode 100644 test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
>
> diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
> index e2171e1b..2702f79f 100644
> --- a/src/lj_opt_fold.c
> +++ b/src/lj_opt_fold.c
> @@ -382,10 +382,10 @@ static uint64_t kfold_int64arith(jit_State *J, uint64_t k1, uint64_t k2,
> case IR_BOR: k1 |= k2; break;
> case IR_BXOR: k1 ^= k2; break;
> case IR_BSHL: k1 <<= (k2 & 63); break;
> - case IR_BSHR: k1 = (int32_t)((uint32_t)k1 >> (k2 & 63)); break;
> - case IR_BSAR: k1 >>= (k2 & 63); break;
> - case IR_BROL: k1 = (int32_t)lj_rol((uint32_t)k1, (k2 & 63)); break;
> - case IR_BROR: k1 = (int32_t)lj_ror((uint32_t)k1, (k2 & 63)); break;
> + case IR_BSHR: k1 >>= (k2 & 63); break;
> + case IR_BSAR: k1 = (uint64_t)((int64_t)k1 >> (k2 & 63)); break;
> + case IR_BROL: k1 = lj_rol(k1, (k2 & 63)); break;
> + case IR_BROR: k1 = lj_ror(k1, (k2 & 63)); break;
> default: lj_assertJ(0, "bad IR op %d", op); break;
> }
> #else
> diff --git a/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua b/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
> new file mode 100644
> index 00000000..6cc0b319
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
> @@ -0,0 +1,74 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate LuaJIT misbehaviour on folding
> +-- for bitshift operations.
> +-- See also,https://github.com/LuaJIT/LuaJIT/issues/1079.
> +
> +local test = tap.test('lj-1079-fix-64-bitshift-folds'):skipcond({
> + ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +local bit = require('bit')
> +
> +test:plan(4)
> +
> +-- Generic function for `bit.ror()`, `bit.rol()`.
> +local function bitop_rotation(bitop)
I would rename arg `bitop` to `bitop_func` to highlight the type
of the value.
> + local r = {}
> + for i = 1, 4 do
> + -- (i & k1) o k2 ==> (i o k2) & (k1 o k2)
> + local int64 = bit.band(i, 7LL)
> + r[i] = tonumber(bitop(int64, 32))
please add comments about magic constants here and below
> + end
> + return r
> +end
> +
> +-- Similar function for `bit.rshift()`.
> +local function bitop_rshift_signed()
> + local r = {}
> + for i = 1, 4 do
> + -- (i & k1) o k2 ==> (i o k2) & (k1 o k2)
> + -- XXX: Use `-i` instead of `i` to prevent other folding due
> + -- to IR difference so the IRs don't match fold rule mask.
> + -- (-i & 7LL) < 1 << 32 => result == 0.
> + local int64 = bit.band(-i, 7LL)
> + r[i] = tonumber(bit.rshift(int64, 32))
> + end
> + return r
> +end
> +
> +-- A little bit different example, which leads to the assertion
> +-- failure due to the incorrect recording.
> +local function bitop_rshift_huge()
> + local r = {}
> + for i = 1, 4 do
> + -- (i & k1) o k2 ==> (i o k2) & (k1 o k2)
> + -- XXX: Need to use cast to the int64_t via `+ 0LL`, see the
> + -- documentation [1] for the details.
> + -- [1]:https://bitop.luajit.org/semantics.html
> + local int64 = bit.band(2 ^ 33 + i, 2 ^ 33 + 0LL)
> + r[i] = tonumber(bit.rshift(int64, 32))
> + end
> + return r
> +end
> +
> +local function test_64bitness(subtest, payload_func, bitop)
> + subtest:plan(1)
> +
> + jit.off()
> + jit.flush()
> + local results_joff = payload_func(bitop)
> + jit.on()
> + -- Reset hotcounters.
> + jit.opt.start('hotloop=1')
> + local results_jon = payload_func(bitop)
> + subtest:is_deeply(results_jon, results_joff,
> + 'same results for VM and JIT for ' .. subtest.name)
> +end
> +
> +test:test('rol', test_64bitness, bitop_rotation, bit.rol)
> +test:test('ror', test_64bitness, bitop_rotation, bit.ror)
> +test:test('rshift signed', test_64bitness, bitop_rshift_signed)
> +test:test('rshift huge', test_64bitness, bitop_rshift_huge)
have you added additional whitespaces intentionally?
> +
> +test:done(true)
[-- Attachment #2: Type: text/html, Size: 6196 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules.
2024-10-08 12:07 ` Sergey Bronnikov via Tarantool-patches
@ 2024-10-08 14:24 ` Sergey Kaplun via Tarantool-patches
2024-10-09 14:29 ` Sergey Bronnikov via Tarantool-patches
2024-10-11 19:12 ` Maxim Kokryashkin via Tarantool-patches
0 siblings, 2 replies; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-10-08 14:24 UTC (permalink / raw)
To: Sergey Bronnikov; +Cc: tarantool-patches
Hi, Sergey!
Thanks for the review!
Fixed your comments and force-pushed the branch.
On 08.10.24, Sergey Bronnikov wrote:
> Hello, Sergey,
>
> thanks for the patch! Please see my comments below.
>
> On 02.10.2024 11:09, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Thanks to Peter Cawley.
> >
> > (cherry picked from commit 9e0437240f1fb4bfa7248f6ec8be0e3181016119)
> >
> > For `IR_BSHR`, `IR_BROL`, `IR_BROR` during `kfold_int64arith()` the left
> > argument is truncated down to 32 bits, which leads to incorrect results
> > if the right argument is >= 32.
> typo: is >= 2,147,483,647
Nice catch! Thanks! :)
I replaced with 2^32.
> >
> > Also, `IR_BSAR` does an unsigned shift rather than a signed shift, but
> > since this case branch is unreachable, it is harmless for now.
> >
> > This patch fixes all misbehaviours (including possible for `IR_BSAR`) to
> > preserve IR semantics.
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#10199
> > ---
> > src/lj_opt_fold.c | 8 +-
> > .../lj-1079-fix-64-bitshift-folds.test.lua | 74 +++++++++++++++++++
> > 2 files changed, 78 insertions(+), 4 deletions(-)
> > create mode 100644 test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
> >
> > diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
> > index e2171e1b..2702f79f 100644
> > --- a/src/lj_opt_fold.c
> > +++ b/src/lj_opt_fold.c
<snipped>
> > diff --git a/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua b/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
> > new file mode 100644
> > index 00000000..6cc0b319
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
> > @@ -0,0 +1,74 @@
> > +local tap = require('tap')
> > +
> > +-- Test file to demonstrate LuaJIT misbehaviour on folding
> > +-- for bitshift operations.
> > +-- See also,https://github.com/LuaJIT/LuaJIT/issues/1079.
> > +
> > +local test = tap.test('lj-1079-fix-64-bitshift-folds'):skipcond({
> > + ['Test requires JIT enabled'] = not jit.status(),
> > +})
> > +
> > +local bit = require('bit')
> > +
> > +test:plan(4)
> > +
> > +-- Generic function for `bit.ror()`, `bit.rol()`.
> > +local function bitop_rotation(bitop)
>
> I would rename arg `bitop` to `bitop_func` to highlight the type
>
> of the value.
Renamed, see the iterational patch below.
>
> > + local r = {}
> > + for i = 1, 4 do
> > + -- (i & k1) o k2 ==> (i o k2) & (k1 o k2)
> > + local int64 = bit.band(i, 7LL)
> > + r[i] = tonumber(bitop(int64, 32))
> please add comments about magic constants here and below
Added the corresponding comment.
> > + end
> > + return r
> > +end
> > +
<snipped>
> > +
> > +test:test('rol', test_64bitness, bitop_rotation, bit.rol)
> > +test:test('ror', test_64bitness, bitop_rotation, bit.ror)
> > +test:test('rshift signed', test_64bitness, bitop_rshift_signed)
> > +test:test('rshift huge', test_64bitness, bitop_rshift_huge)
> have you added additional whitespaces intentionally?
Yes, I rearranged them a bit to avoid confusion. See the iterative patch
below.
===================================================================
diff --git a/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua b/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
index 6cc0b319..28383bf9 100644
--- a/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
+++ b/test/tarantool-tests/lj-1079-fix-64-bitshift-folds.test.lua
@@ -13,12 +13,15 @@ local bit = require('bit')
test:plan(4)
-- Generic function for `bit.ror()`, `bit.rol()`.
-local function bitop_rotation(bitop)
+local function bitop_rotation(bitop_func)
local r = {}
for i = 1, 4 do
-- (i & k1) o k2 ==> (i o k2) & (k1 o k2)
+ -- XXX: Don't use named constants here to match folding rules.
+ -- `7LL` is just some mask, that doesn't change the `i` value.
+ -- `32` is used for the half bit-width rotation.
local int64 = bit.band(i, 7LL)
- r[i] = tonumber(bitop(int64, 32))
+ r[i] = tonumber(bitop_func(int64, 32))
end
return r
end
@@ -52,23 +55,23 @@ local function bitop_rshift_huge()
return r
end
-local function test_64bitness(subtest, payload_func, bitop)
+local function test_64bitness(subtest, payload_func, bitop_func)
subtest:plan(1)
jit.off()
jit.flush()
- local results_joff = payload_func(bitop)
+ local results_joff = payload_func(bitop_func)
jit.on()
-- Reset hotcounters.
jit.opt.start('hotloop=1')
- local results_jon = payload_func(bitop)
+ local results_jon = payload_func(bitop_func)
subtest:is_deeply(results_jon, results_joff,
'same results for VM and JIT for ' .. subtest.name)
end
-test:test('rol', test_64bitness, bitop_rotation, bit.rol)
-test:test('ror', test_64bitness, bitop_rotation, bit.ror)
test:test('rshift signed', test_64bitness, bitop_rshift_signed)
test:test('rshift huge', test_64bitness, bitop_rshift_huge)
+test:test('rol', test_64bitness, bitop_rotation, bit.rol)
+test:test('ror', test_64bitness, bitop_rotation, bit.ror)
test:done(true)
===================================================================
> > +
> > +test:done(true)
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules.
2024-10-08 14:24 ` Sergey Kaplun via Tarantool-patches
@ 2024-10-09 14:29 ` Sergey Bronnikov via Tarantool-patches
2024-10-11 19:12 ` Maxim Kokryashkin via Tarantool-patches
1 sibling, 0 replies; 10+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2024-10-09 14:29 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
[-- Attachment #1: Type: text/plain, Size: 225 bytes --]
Hi, Sergey,
thanks for the fixes! LGTM
On 08.10.2024 17:24, Sergey Kaplun wrote:
> Hi, Sergey!
> Thanks for the review!
> Fixed your comments and force-pushed the branch.
>
> On 08.10.24, Sergey Bronnikov wrote:
>
<snipped>
[-- Attachment #2: Type: text/html, Size: 678 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 1/2] Fix bit op coercion in DUALNUM builds.
2024-10-02 8:09 ` [Tarantool-patches] [PATCH luajit 1/2] Fix bit op coercion in DUALNUM builds Sergey Kaplun via Tarantool-patches
2024-10-08 10:12 ` Sergey Bronnikov via Tarantool-patches
@ 2024-10-11 19:08 ` Maxim Kokryashkin via Tarantool-patches
1 sibling, 0 replies; 10+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2024-10-11 19:08 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
Hi, Sergey!
Thanks for the patch!
LGTM
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules.
2024-10-08 14:24 ` Sergey Kaplun via Tarantool-patches
2024-10-09 14:29 ` Sergey Bronnikov via Tarantool-patches
@ 2024-10-11 19:12 ` Maxim Kokryashkin via Tarantool-patches
1 sibling, 0 replies; 10+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2024-10-11 19:12 UTC (permalink / raw)
To: Sergey Kaplun; +Cc: tarantool-patches
Hi, Sergey!
Thanks for the fixes!
LGTM
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Tarantool-patches] [PATCH luajit 0/2] Fixes for 64 bit operands of the bit library
2024-10-02 8:09 [Tarantool-patches] [PATCH luajit 0/2] Fixes for 64 bit operands of the bit library Sergey Kaplun via Tarantool-patches
2024-10-02 8:09 ` [Tarantool-patches] [PATCH luajit 1/2] Fix bit op coercion in DUALNUM builds Sergey Kaplun via Tarantool-patches
2024-10-02 8:09 ` [Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules Sergey Kaplun via Tarantool-patches
@ 2024-10-18 15:17 ` Sergey Kaplun via Tarantool-patches
2 siblings, 0 replies; 10+ messages in thread
From: Sergey Kaplun via Tarantool-patches @ 2024-10-18 15:17 UTC (permalink / raw)
To: Maxim Kokryashkin, Sergey Bronnikov; +Cc: tarantool-patches
I've applied the patchset into all long-term branches in
tarantool/luajit and bumped a new version in master [1], release/3.2 [2]
and release/2.11 [3].
[1]: https://github.com/tarantool/tarantool/pull/10712
[2]: https://github.com/tarantool/tarantool/pull/10713
[3]: https://github.com/tarantool/tarantool/pull/10714
--
Best regards,
Sergey Kaplun
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-10-18 15:18 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-10-02 8:09 [Tarantool-patches] [PATCH luajit 0/2] Fixes for 64 bit operands of the bit library Sergey Kaplun via Tarantool-patches
2024-10-02 8:09 ` [Tarantool-patches] [PATCH luajit 1/2] Fix bit op coercion in DUALNUM builds Sergey Kaplun via Tarantool-patches
2024-10-08 10:12 ` Sergey Bronnikov via Tarantool-patches
2024-10-11 19:08 ` Maxim Kokryashkin via Tarantool-patches
2024-10-02 8:09 ` [Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules Sergey Kaplun via Tarantool-patches
2024-10-08 12:07 ` Sergey Bronnikov via Tarantool-patches
2024-10-08 14:24 ` Sergey Kaplun via Tarantool-patches
2024-10-09 14:29 ` Sergey Bronnikov via Tarantool-patches
2024-10-11 19:12 ` Maxim Kokryashkin via Tarantool-patches
2024-10-18 15:17 ` [Tarantool-patches] [PATCH luajit 0/2] Fixes for 64 bit operands of the bit library 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