Re: [Tarantool-patches] [PATCH luajit] Fix ABC FOLD rule with constants.

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

Hello, Sergey!
Thanks for the review!
Please consider my answers below.

On 18.11.23, Sergey Bronnikov wrote:
> Hello, Sergey!
> 
> thanks for the patch! LGTM, see minor comments below.
> 
> Sergey
> 
> On 11/13/23 18:05, Sergey Kaplun wrote:
> > From: Mike Pall <mike>
> >
> > Reported by XmiliaH.
> >
> > (cherry-picked from commit c8bcf1e5fb8eb72c7e35604fdfd27bba512761bb)
> >
> > `fold_abc_k()` doesn't patch the first ABC check when the right constant
> > operand is negative. This leads to out-of-bounds access from the array
> > on a trace. This patch casts to uint32_t the operands to compare. If the
> > right IR contains a negative integer, the second IR will always be
> > patched. Also, because the ABC check on the trace is unordered, this
> > guard will always fail.
> >
> > Also, this fold rule creates new instructions that reference operands
> IR output would be useful in a test, what do you think?

I am not really sure about that (if I did, I would add it).
The mention of missed IRs sounds like a good compromise.
Anyone interested in the output dump can observe it by running test from
the command line.

> > across PHIs. This opens the room for other optimizations (like DCE), so
> > some guards become eliminated, and we use out-of-bounds access from the
> > array part of the table on trace. This patch adds the missing
> > `PHIBARRIER()` check.
> >
> > Sergey Kaplun:
> > * added the description and the test for the problem
> >
> > Part of tarantool/tarantool#9145
> > ---
> > Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-794-abc-fold-constants
> > Tarantool PR: https://github.com/tarantool/tarantool/pull/9364
> > Related issues:
> > * https://github.com/LuaJIT/LuaJIT/issues/794
> > * https://github.com/tarantool/tarantool/issues/9145
> >
> >   src/lj_opt_fold.c                             |  5 +-
> >   .../lj-794-abc-fold-constants.test.lua        | 85 +++++++++++++++++++
> >   2 files changed, 88 insertions(+), 2 deletions(-)
> >   create mode 100644 test/tarantool-tests/lj-794-abc-fold-constants.test.lua

<snipped>

> > diff --git a/test/tarantool-tests/lj-794-abc-fold-constants.test.lua b/test/tarantool-tests/lj-794-abc-fold-constants.test.lua
> > new file mode 100644
> > index 00000000..f8609933
> > --- /dev/null
> > +++ b/test/tarantool-tests/lj-794-abc-fold-constants.test.lua
> > @@ -0,0 +1,85 @@
> > +local tap = require('tap')
> > +
> > +-- Test file to demonstrate LuaJIT's incorrect fold optimization
> > +-- for Array Bound Check for constants.
> > +-- ABC(asize, k1), ABC(asize k2) ==> ABC(asize, max(k1, k2)).
> > +-- See also https://github.com/LuaJIT/LuaJIT/issues/794.
> > +
> > +local test = tap.test('lj-794-abc-fold-constants'):skipcond({
> > +  ['Test requires JIT enabled'] = not jit.status(),
> > +})
> > +
> > +local MAGIC_UNUSED = 42
> 
> AFAIK we put all test-related stuff after "test:plan".
> 
> Feel free to ignore.

Fixed, thanks!
See the iterative patch below.

===================================================================
diff --git a/test/tarantool-tests/lj-794-abc-fold-constants.test.lua b/test/tarantool-tests/lj-794-abc-fold-constants.test.lua
index c69d395b..53e4d2eb 100644
--- a/test/tarantool-tests/lj-794-abc-fold-constants.test.lua
+++ b/test/tarantool-tests/lj-794-abc-fold-constants.test.lua
@@ -9,9 +9,10 @@ local test = tap.test('lj-794-abc-fold-constants'):skipcond({
   ['Test requires JIT enabled'] = not jit.status(),
 })
 
-local MAGIC_UNUSED = 42
 test:plan(2)
 
+local MAGIC_UNUSED = 42
+
 local function abc_check_sign()
   local tab = {MAGIC_UNUSED}
   local return_value = 0
===================================================================

Branch is force-pushed.

> 
> > +test:plan(2)
> > +
> > +local function abc_check_sign()
> > +  local tab = {MAGIC_UNUSED}
> > +  local return_value = 0
> > +  local abc_limit = 1
> > +  -- No need to run the loop on the first call. We will take
> > +  -- the side exit anyway.
> > +  for i = 1, 3 do
> > +    -- Add an additional ABC check to be merged with.
> > +    if i > 1 then
> > +      -- luacheck: ignore
> > +      return_value = tab[1]
> > +      return_value = tab[abc_limit]
> > +      -- XXX: Just use some negative number.
> > +      abc_limit = -1000000
> 
> With -1 works too, I would replace -10^6 with -1 for simplification.

Not always (I tried). I prefer to use the value from the reproducer that
fails stable.
Ignore for now.

> 
> 
> 
> <snipped>
> 

-- 
Best regards,
Sergey Kaplun