[Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules.
Sergey Kaplun
skaplun at tarantool.org
Tue Oct 8 17:24:23 MSK 2024
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
More information about the Tarantool-patches
mailing list