Re: [Tarantool-patches] [PATCH luajit v1 1/5] build: introduce option LUAJIT_ENABLE_TABLE_BUMP

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

Hi, Maxim!
Thanks for the review!
Fixed your comments. See the iterative patch below.

On 05.05.24, Maxim Kokryashkin wrote:
> Hi, Sergey!
> Thanks for the patch!
> LGTM, except for a few nits below.
> On Mon, Apr 22, 2024 at 11:49:54AM UTC, Sergey Kaplun wrote:
> > This option enables table bump optimization if sink optimization is
> > enabled. The table bump optimization patches the bytecodes with a table
> > allocation on the trace recording if the recorded trace exceeds the size
> > of the allocated table. This optimization still has some bugs, so it is
> > disabled by default. For more details, see the comment in
> > <CMakeLists.txt>.
> >
> > Needed for tarantool/tarantool#9924
> > ---
> >  CMakeLists.txt | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/CMakeLists.txt b/CMakeLists.txt
> > index 2355ce17..52014296 100644
> > --- a/CMakeLists.txt
> > +++ b/CMakeLists.txt
> > @@ -307,6 +307,23 @@ if(LUAJIT_ENABLE_COVERAGE)
> >    include(CodeCoverage)
> >  endif()
> >
> > +# Enable table bump optimization. This optimization patches the
> > +# bytecodes with a table allocation on the trace recording if the
> Typo: s/bytecodes/bytecode/

Fixed.

> > +# recorded trace exceeds the size of the allocated table.
> > +# This optimization still has some bugs, so it is disabled by
> > +# default. See also: https://github.com/LuaJIT/LuaJIT/issues/606.
> > +option(LUAJIT_ENABLE_TABLE_BUMP "Enable table bump optimization" OFF)
> > +if(LUAJIT_ENABLE_TABLE_BUMP)
> > +  # Within table bump optimization enabled (and due to our
> Typo: s/Within/With/

Fixed.

> > +  # modification related to metrics), some offsets in `GG_State`
> > +  # stop fit in 12bit immediate. Hence, the build failed due to
> > +  # the DASM error (DASM_S_RANGE_I).
> > +  if(CMAKE_SYSTEM_PROCESSOR STREQUAL "aarch64")
> > +    message(FATAL_ERROR "Table Bump optimization is unsupported for aarch64")
> Typo: s/Bump/bump/

Fixed.

===================================================================
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 52014296..ce8d0311 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -308,18 +308,18 @@ if(LUAJIT_ENABLE_COVERAGE)
 endif()
 
 # Enable table bump optimization. This optimization patches the
-# bytecodes with a table allocation on the trace recording if the
+# bytecode with a table allocation on the trace recording if the
 # recorded trace exceeds the size of the allocated table.
 # This optimization still has some bugs, so it is disabled by
 # default. See also: https://github.com/LuaJIT/LuaJIT/issues/606.
 option(LUAJIT_ENABLE_TABLE_BUMP "Enable table bump optimization" OFF)
 if(LUAJIT_ENABLE_TABLE_BUMP)
-  # Within table bump optimization enabled (and due to our
+  # With table bump optimization enabled (and due to our
   # modification related to metrics), some offsets in `GG_State`
-  # stop fit in 12bit immediate. Hence, the build failed due to
-  # the DASM error (DASM_S_RANGE_I).
+  # stop fitting in 12bit immediate. Hence, the build failed due
+  # to the DASM error (`DASM_S_RANGE_I`).
   if(CMAKE_SYSTEM_PROCESSOR STREQUAL "aarch64")
-    message(FATAL_ERROR "Table Bump optimization is unsupported for aarch64")
+    message(FATAL_ERROR "Table bump optimization is unsupported for aarch64")
   endif()
   AppendFlags(TARGET_C_FLAGS -DLUAJIT_ENABLE_TABLE_BUMP)
 endif()
===================================================================

-- 
Best regards,
Sergey Kaplun