<HTML><BODY><div>Hi, Sergey!</div><div>Thanks for the patch!</div><div>LGTM, except for a few nits below.</div><div> </div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div id=""><div class="js-helper js-readmsg-msg"><div><div id="style_16746732850218582333_BODY">From: Mike Pall <mike><br><br>Reported by caohongqing.<br><br>(cherry picked from commit 5bf0da3d7c02f9959fa3a9fb721e0565137b70c8)<br><br>If there is high register pressure, and there are almost all registers<br>in use during the aforementioned assembling, the same register is chosen<br>as the one holding the given stack slot and the one holding the constant<br>value for the type comparison. As the result we get the following<br>assertion guard check in assembly:<br>| cmp x0, x0, lsr #32<br>| bne ->0<br>Which is always false.<br><br>This happens because the `tmp` register (used for loading constant type<br>to compare in assertion guard) is scratched from `gpr` register set, but<br>not from `allow` set, which is used during picking the register for slot<br>loading (at the begging `allow` and `gpr` sets are the same).<br><br>This patch changes `allow` set to `gpr` to fix the issue.<br><br>Sergey Kaplun:<br>* added the description and the test for the problem<br><br>Part of tarantool/tarantool#8069<br>---<br><br>Branch: <a href="https://github.com/tarantool/luajit/tree/skaplun/or-94-fix-bool-ahuvload" target="_blank">https://github.com/tarantool/luajit/tree/skaplun/or-94-fix-bool-ahuvload</a><br>PR: <a href="https://github.com/tarantool/tarantool/pull/8214" target="_blank">https://github.com/tarantool/tarantool/pull/8214</a><br>Issues:<br>* <a href="https://github.com/tarantool/tarantool/issues/8069" target="_blank">https://github.com/tarantool/tarantool/issues/8069</a><br>* <a href="https://github.com/openresty/luajit2/pull/94" target="_blank">https://github.com/openresty/luajit2/pull/94</a><br><br> src/lj_asm_arm64.h | 2 +-<br> .../or-94-arm64-ir-ahuvload-bool.test.lua | 145 ++++++++++++++++++<br> 2 files changed, 146 insertions(+), 1 deletion(-)<br> create mode 100644 test/tarantool-tests/or-94-arm64-ir-ahuvload-bool.test.lua<br><br>diff --git a/src/lj_asm_arm64.h b/src/lj_asm_arm64.h<br>index 4aeb51f3..30d34ca1 100644<br>--- a/src/lj_asm_arm64.h<br>+++ b/src/lj_asm_arm64.h<br>@@ -1055,7 +1055,7 @@ static void asm_ahuvload(ASMState *as, IRIns *ir)<br> emit_n(as, (A64I_CMNx^A64I_K12) | A64F_U12(1), tmp);<br> } else {<br> emit_nm(as, A64I_CMPx | A64F_SH(A64SH_LSR, 32),<br>- ra_allock(as, (irt_toitype(ir->t) << 15) | 0x7fff, allow), tmp);<br>+ ra_allock(as, (irt_toitype(ir->t) << 15) | 0x7fff, gpr), tmp);<br> }<br> if (ofs & FUSE_REG)<br> emit_dnm(as, (A64I_LDRx^A64I_LS_R)|A64I_LS_UXTWx|A64I_LS_SH, tmp, idx, (ofs & 31));<br>diff --git a/test/tarantool-tests/or-94-arm64-ir-ahuvload-bool.test.lua b/test/tarantool-tests/or-94-arm64-ir-ahuvload-bool.test.lua<br>new file mode 100644<br>index 00000000..a9b0071c<br>--- /dev/null<br>+++ b/test/tarantool-tests/or-94-arm64-ir-ahuvload-bool.test.lua<br>@@ -0,0 +1,145 @@<br>+local tap = require('tap')<br>+-- Test file to demonstrate the incorrect JIT assembling<br>+-- for IR_{AHUV}LOAD specialized to boolean.<br>+-- See also: <a href="https://github.com/openresty/luajit2/pull/94" target="_blank">https://github.com/openresty/luajit2/pull/94</a>.<br>+<br>+-- If there is high register pressure, and there are almost<br>+-- all registers in use during the aforementioned assembling,<br>+-- the same register is chosen as the one holding the given<br>+-- stack slot and the one holding the constant value for the type<br>+-- comparison. As the result we get the following assertion<br>+-- guard check in assembly:<br>+-- | cmp x0, x0, lsr #32<br>+-- | bne ->0<br>+-- Which is always false.<br>+local ffi = require('ffi')<br>+-- Need for code generation.<br>+_G.ffi = ffi<br>+<br>+local traceinfo = require('jit.util').traceinfo<br>+<br>+-- Each payload will be recording with the corresponding IR.<br>+local TESTS = {<br>+ {irname = 'ALOAD', payload = 'aload()'},<br>+ {irname = 'HLOAD', payload = 'hload()'},<br>+ {irname = 'ULOAD', payload = 'uload()'},<br>+ {irname = 'VLOAD', payload = '...'},<br>+}<br>+local N_TESTS = #TESTS<br>+<br>+local test = tap.test('or-94-arm64-ir-ahuvload-bool')<br>+test:plan(N_TESTS)<br>+<br>+-- Functions to be inlined on trace to generate different<br>+-- types of IRs (ALOAD, HLOAD, ULOAD).<br>+-- Declare as global for code generation.<br>+local arr = {true}<br>+local function aload()<br>+ return arr[1]<br>+end<br>+_G.aload = aload<br>+<br>+local h = {data = true}<br>+local function hload()<br>+ local boolvalue = h.data<br>+ return boolvalue<br>+end<br>+_G.hload = hload<br>+<br>+do<br>+ local upvalue = true<br>+ local function uload()<br>+ return upvalue<br>+ end<br>+ -- Make upvalue muttable. Not really need to return this</div></div></div></div></blockquote><div>Typo: s/muttable/mutable/</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>+ -- function.<br>+ local function _()<br>+ upvalue = not upvalue<br>+ end<br>+ _G.uload = uload<br>+end<br>+<br>+-- This function generate code like the following:</div></div></div></div></blockquote><div>Typo: s/generate/generates/</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>+-- | local test_f(...)<br>+-- | local r<br>+-- | local rup1<br>+-- | --[[...]]<br>+-- | for _ = 1, 4 do<br>+-- | r1 = ffi.cast("int", 1)<br>+-- | --[[...]]<br>+-- | r = main_payload()<br>+-- | rup1 = r1<br>+-- | --[[...]]<br>+-- | end<br>+-- | end<br>+-- | return test_f<br>+-- Those `rn` variables before and after `main_payload` are<br>+-- required to generate enough register pressure (for GPR). Amount<br>+-- of repeats is empirical.<br>+-- Additional `test_f(...)` wrapper is needed for IR_VLOAD usage,<br>+-- when `main_payload` is just `...`.<br>+local function generate_payload(n_fillers, main_payload)<br>+ local code_chunk = 'local function test_f(...)\n'<br>+ code_chunk = code_chunk .. 'local r\n'<br>+ for i = 1, n_fillers do<br>+ code_chunk = code_chunk .. ('local rup%d\n'):format(i)<br>+ end<br>+ code_chunk = code_chunk .. 'for _ = 1, 4 do\n'<br>+ for i = 1, n_fillers do<br>+ code_chunk = code_chunk ..<br>+ ('local r%d = ffi.cast("int", %d)\n'):format(i, i)<br>+ end<br>+ code_chunk = code_chunk .. 'r = ' .. main_payload .. '\n'<br>+ for i = 1, n_fillers do<br>+ code_chunk = code_chunk .. ('rup%d = r%d\n'):format(i, i)<br>+ end<br>+ code_chunk = code_chunk .. 'end\nend\n'<br>+ code_chunk = code_chunk .. 'return test_f'<br>+ local f, err = loadstring(code_chunk, 'test_function')<br>+ assert(type(f) == 'function', err)<br>+ f = f()<br>+ assert(type(f) == 'function', 'returned generated value is not a function')<br>+ return f<br>+end</div></div></div></div></blockquote><div>That section is really hard to read, is there any way to make it more</div><div>readable? I believe even slight reformatting might help.</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>+<br>+-- Disable sink optimization to allocate more registers in a<br>+-- "convenient" way. 'hotexit' option is required to be sure that<br>+-- we will start a new trace on false-positive guard assertion.</div></div></div></div></blockquote><div>Typo: s/on false-positive/on a false-positive/</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>+-- The new trace contains the same IR and so the same assertion<br>+-- guard. This trace will be executed, assertion guard failed<br>+-- again and the new third trace will be recorded. This trace will</div></div></div></div></blockquote><div>Typo: s/assertion guard failed again/the assertion guard will fail again/</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>+-- be the last one to record as far as iterations over cycle are<br>+-- finished and we returning from the function. The report of</div></div></div></div></blockquote><div>Typo: s/we returning/we are returning/</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><div class="js-helper js-readmsg-msg"><div><div>+-- `jit.dump` before the patch is the following:<br>+-- | TRACE 1 start "test_function":29<br>+-- | TRACE 1 stop -> loop<br>+-- | TRACE 1 exit 0<br>+-- | TRACE 2 start 1/0 "test_function":30<br>+-- | TRACE 2 stop -> loop<br>+-- | TRACE 2 exit 0<br>+-- | TRACE 3 start 2/0 "test_function":30<br>+-- | TRACE 3 stop -> return<br>+-- On the other hand, after the patch we will have only 2 traces:<br>+-- the main one, and the second is recorded on exit from loop.<br>+-- Hence, the report of `jit.dump` will be the following:<br>+-- | TRACE 1 start "test_function":29<br>+-- | TRACE 1 stop -> loop<br>+-- | TRACE 1 exit 3<br>+-- | TRACE 2 start 1/3 "test_function":84<br>+-- | TRACE 2 stop -> return<br>+jit.opt.start('hotloop=1', 'hotexit=1', '-sink')<br>+-- Prevent random hotcount to be sure that the cycle is compiled<br>+-- first.<br>+jit.off()<br>+jit.flush()<br>+for i = 1, N_TESTS do<br>+ local f = generate_payload(26, TESTS[i].payload)<br>+ jit.on()<br>+ -- Argument is needed only for IR_VLOAD.<br>+ f(true)<br>+ jit.off()<br>+ test:ok(not traceinfo(3), 'not recorded sidetrace for IR_' .. TESTS[i].irname)<br>+ jit.flush()<br>+end<br>+<br>+os.exit(test:check() and 0 or 1)<br>--<br>2.34.1</div></div></div></div></blockquote><div><div>--<br>Best regards,</div><div>Maxim Kokryashkin</div></div></div></blockquote></BODY></HTML>