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

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

Hi, Maxim!
Thanks for the review!
Please consider my answers below.

On 17.11.23, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> Please consider my comments below.
> 
> On Mon, Nov 13, 2023 at 06:05:01PM +0300, 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
> It would be right to paraphrase this sentence like this:
> "This patch casts the operands to uint32_t for comparison."

Replaced, thanks.

> > 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
> > 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
> >
> > diff --git a/src/lj_opt_fold.c b/src/lj_opt_fold.c
> > index 944a9ecc..6175f7c1 100644
> > --- a/src/lj_opt_fold.c
> > +++ b/src/lj_opt_fold.c
> <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 @@
> <snipped>
> 
> > +-- Now test the second issue, when ABC optimization applies for
> > +-- operands across PHIs.
> > +
> > +-- XXX: Reset hotcounters to avoid collisions.
> > +jit.opt.start('hotloop=1')
> > +
> > +local tab_array = {}
> > +local small_tab = {MAGIC_UNUSED}
> > +local full_tab = {}
> > +
> > +-- First, create tables with different asizes, to be used in PHI.
> > +-- Create a large enough array part for the noticeable
> > +-- out-of-bounds access.
> > +for i = 1, 8 do
> > +  full_tab[i] = MAGIC_UNUSED
> > +end
> > +
> > +-- We need 5 iterations to execute both the variant and the
> > +-- invariant parts of the trace below.
> > +for i = 1, 5 do
> > +  -- On the 3rd iteration, the recording is started.
> > +  if i > 3 then
> > +    tab_array[i] = small_tab
> > +  else
> > +    tab_array[i] = full_tab
> > +  end
> > +end
> > +
> > +local result
> > +local alias_tab = tab_array[1]
> > +-- Compile a trace.
> > +-- Run 5 iterations to execute both the variant and the invariant
> > +-- parts.
> > +for i = 1, 5 do
> > +  local local_tab = alias_tab
> > +  alias_tab = tab_array[i]
> > +  -- Additional ABC check to fold.
> > +  -- luacheck: ignore
> > +  result = alias_tab[1]
> > +  result = local_tab[8]
> > +end
> 
> The black magic that happens here with tables is hard to understand.
> Please drop a comment with a detailed explanations for why do we need
> this complex `tab_array` construction and what effects does this have on
> IRs.

Added the following comment for clarification:
Also, renaming `local_tab` -> `previous_tab` to avoid confusion and
emphasize that the table from the previous iteration, which ABC check IR
is used.

===================================================================
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 f8609933..c69d395b 100644
--- a/test/tarantool-tests/lj-794-abc-fold-constants.test.lua
+++ b/test/tarantool-tests/lj-794-abc-fold-constants.test.lua
@@ -55,6 +55,12 @@ for i = 1, 8 do
   full_tab[i] = MAGIC_UNUSED
 end
 
+-- Now, store these tables in the array. The PHI should be used in
+-- the trace to distinguish asizes from the variant and the
+-- invariant parts of the loop for the future ABC check.
+-- Nevertheless, before the patch, the ABC IR and the
+-- corresponding PHI are folded via optimization. This leads to
+-- incorrect behaviour.
 -- We need 5 iterations to execute both the variant and the
 -- invariant parts of the trace below.
 for i = 1, 5 do
@@ -72,12 +78,12 @@ local alias_tab = tab_array[1]
 -- Run 5 iterations to execute both the variant and the invariant
 -- parts.
 for i = 1, 5 do
-  local local_tab = alias_tab
+  local previous_tab = alias_tab
   alias_tab = tab_array[i]
   -- Additional ABC check to fold.
   -- luacheck: ignore
   result = alias_tab[1]
-  result = local_tab[8]
+  result = previous_tab[8]
 end
 
 test:is(result, nil, 'correct ABC constant rule across PHI')
===================================================================

> > +
> > +test:is(result, nil, 'correct ABC constant rule across PHI')
> > +
> > +test:done(true)
> > --
> > 2.42.0
> >

-- 
Best regards,
Sergey Kaplun