Hi! Thanks for the fixes, LGTM now. Sergos > On 29 Sep 2022, at 12:58, Maxim Kokryashkin wrote: > > Hi, Sergos! > Thanks for the questions! > Please consider my answers amd changes below. > > > LuaJIT narrowing optimization during BC_UNM recording may ignore > > information about sign of zero for integer types of IR. So far the > > resulting value on a trace is not the same as for the interpreter. > > I didn’t get the point - how is it detected, otherwise than tostring()? > If so - should we change the tostring() instead? > Otherwise - we need a test that exposes this difference > I’ve changed the tests, so it’s now more clear that zero sign can affect arithmetic. > Branch is force-pushed. > Here is the diff: > =============================================== > --- a/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua > +++ b/test/tarantool-tests/gh-6976-narrowing-of-unary-minus.test.lua > @@ -2,7 +2,7 @@ > local test = tap.test('gh-6976-narrowing-of-unary-minus') > test:plan(2) > > -jit.opt.start('hotloop=1', 'hotexit=1') > +jit.opt.start('hotloop=1') > > local function check(routine) > jit.off() > @@ -20,32 +20,29 @@ > return true > end > > -test:ok( > - check( > - function() > - local res = require('table.new')(3, 0) > - for _ = 1, 3 do > - local zero = 0 > - zero = -zero > - table.insert(res, tostring(zero)) > - end > - return res > - end > - ), > - 'incorrect recording for zero' > -) > - > -test:ok( > - check( > - function() > - local res = require('table.new')(3, 0) > - for i = 2, 0, -1 do > - table.insert(res, tostring(-i)) > - end > - return res > - end > - ), > - 'assertion guard fail' > -) > +test:ok(check(function() > + -- We use `table.new()` here to avoid trace > + -- exits due to table rehashing. > + local res = require('table.new')(3, 0) > + for _ = 1, 3 do > + local zero = 0 > + zero = -zero > + -- There is no difference between 0 and -0 from > + -- arithmetic perspective, unless you try to divide > + -- something by them. > + -- `1 / 0 = inf` and `1 / -0 = -inf` > + table.insert(res, 1 / zero) > + end > + return res > +end), 'incorrect recording for zero') > + > +test:ok(check(function() > + -- See the comment about `table.new()` above. > + local res = require('table.new')(3, 0) > + for i = 2, 0, -1 do > + table.insert(res, 1 / -i) > + end > + return res > +end),'assertion guard fail') > > os.exit(test:check() and 0 or 1) > =============================================== > > > > This patch fixes the non-DUALNUM mode behaviour. When the zero value is > > identified during recording it should be cast to number so IR_CONV is > > emitted. Also, this patch adds assertion guard checking that value on > > which operation of unary minus is performed isn't zero. > > Does it mean I will exit the trace every time I met a `x = 0; x = -x` in it? > No, that assertion guard takes you back to the interpreter only if a > trace for unary minus was recorded considering `x` as a non-zero value, > and at some point in this trace `x` became zero. ok, it looks reasonable. > > > Best regards, > Maxim Kokryashkin >