Re: [Tarantool-patches] [PATCH luajit 2/2] FFI: Fix 64 bit shift fold rules.

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

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