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

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

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."
> 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.
> +
> +test:is(result, nil, 'correct ABC constant rule across PHI')
> +
> +test:done(true)
> --
> 2.42.0
>