Re: [Tarantool-patches] [PATCH luajit 2/2] Fix canonicalization of +-0.0 keys for IR_NEWREF.

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

Hi, Maxim!
Thanks for the review!
Fixed your comments and force-pushed the branch.

On 15.05.23, Maxim Kokryashkin wrote:
> 
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for a few nits regarding the commit message.
>  
> > 
> >>From: Mike Pall <mike>
> >>
> >>Reported by Sergey Kaplun.
> >>
> >>(cherry picked from commit 96fc114a7a3be3fd2c227d5a0ac53aa50cfb85d1)
> >>
> >>This commit is a follow-up for the commit
> >>f067cf638cf8987ab3b6db372d609a5982e458b5 ("Fix narrowing of unary
> >>minus."). Since this commit -0 IR constant is stored as well as +0
> >Typo: s/as well as/as well as the/

Fixed.

> >>constant on the trace. Since IR NEWREF keys don't canonicalizied for -0
> >Typo: s/don’t canonicalized/don’t get canonicalized/

Changed to "aren't canonicalizied", thanks!

> >>opposed of IR HREFK, it may lead to inconsistencies during trace
> >Typo: s/opposed of/as opposed to/

Fixed! Thanks!

> >>recording.
> >>
> >>In particular, since -0 and 0 are different IR constants, alias analysis
> >>declares that they can't be aliased during folding optimization.
> >>Therefore:
> >>1) For the IR TNEW we have non-nil value to lookup from the table via
> >>   HLOAD, when only nil lookup is expected due to alias analysis.
> >>2) For the IR TDUP we have non-nil value to lookup from the table via
> >>   HLOAD, but the template table has no 0 field initiated as far as -0
> >>   isn't folding to 0 during parsing (see `bcemit_unop()` in
> >>   <src/lj_parse.c>).
> >>These cases lead to the assertion failures in `fwd_ahload()`.
> >Typo: s/the assertion/assertion/

Fixed!

> >>
> >>This patch adds the aforementioned canonicalization.
> >>
> >>Sergey Bronnikov:
> >>* reported the original issue for the TDUP IR
> >>
> >>Sergey Kaplun:
> >>* added the description and the test for the problem
> >>
> >>Part of tarantool/tarantool#8516
> >>---
> >>
> >>Side note: I don't mention the 981 issue by intend -- I don't want to
> >>bother Mike with force pushes:). I suppose Igor should add this line (if
> >>he wants) went this commit will be cherry-picked to our master branch
> >>(a.k.a. tarantool).
> >>
> >> src/lj_record.c | 2 +
> >> .../tarantool-tests/lj-981-folding-0.test.lua | 57 +++++++++++++++++++
> >> 2 files changed, 59 insertions(+)
> >> create mode 100644 test/tarantool-tests/lj-981-folding-0.test.lua
> >>
> >>diff --git a/src/lj_record.c b/src/lj_record.c
> >>index 9e2e1d9e..cc44db8d 100644
> >>--- a/src/lj_record.c
> >>+++ b/src/lj_record.c

<snipped>

> >>diff --git a/test/tarantool-tests/lj-981-folding-0.test.lua b/test/tarantool-tests/lj-981-folding-0.test.lua
> >>new file mode 100644
> >>index 00000000..251da24d
> >>--- /dev/null
> >>+++ b/test/tarantool-tests/lj-981-folding-0.test.lua

<snipped>

> >>2.34.1
> >--
> >Best regards,
> >Maxim Kokryashkin

-- 
Best regards,
Sergey Kaplun