* [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
* 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 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
* [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 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 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