Skip to content

Commit

Permalink
Limit CSE for IR_CARG to fix loop optimizations.
Browse files Browse the repository at this point in the history
Thanks to Peter Cawley.

(cherry picked from commit 3bdc649)

`IR_CALLXS` for the vararg function contains `IR_CARG(fptr, ctid)` as
the second operand. The `loop_emit_phi()` scans only the first operand
of the IR, so the second is not marked as PHI. In this case, when the IR
appears in both the invariant and variant parts of the loop, CSE may
remove it and thus lead to incorrect emitting results.

This patch tweaks the CSE rules to avoid CSE across the `IR_LOOP`.

Sergey Kaplun:
* added the description and the test for the problem

Part of tarantool/tarantool#10199

Reviewed-by: Sergey Bronnikov <[email protected]>
Reviewed-by: Maxim Kokryashkin <[email protected]>
Signed-off-by: Sergey Kaplun <[email protected]>
  • Loading branch information
Mike Pall authored and Buristan committed Oct 17, 2024
1 parent f0bc089 commit b52fe97
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 0 deletions.
11 changes: 11 additions & 0 deletions src/lj_opt_fold.c
Original file line number Diff line number Diff line change
Expand Up @@ -2406,6 +2406,17 @@ LJFOLD(XSNEW any any)
LJFOLD(BUFHDR any any)
LJFOLDX(lj_ir_emit)

/* -- Miscellaneous ------------------------------------------------------- */

LJFOLD(CARG any any)
LJFOLDF(cse_carg)
{
TRef tr = lj_opt_cse(J);
if (tref_ref(tr) < J->chain[IR_LOOP]) /* CSE across loop? */
return EMITFOLD; /* Raw emit. Assumes fins is left intact by CSE. */
return tr;
}

/* ------------------------------------------------------------------------ */

/* Every entry in the generated hash table is a 32 bit pattern:
Expand Down
56 changes: 56 additions & 0 deletions test/tarantool-tests/lj-1244-missing-phi-carg.test.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
local ffi = require('ffi')
local table_new = require('table.new')

-- Test file to demonstrate LuaJIT incorrect behaviour for
-- recording the FFI call to the vararg function. See also:
-- https://github.com/LuaJIT/LuaJIT/issues/1244.
local tap = require('tap')
local test = tap.test('lj-1244-missing-phi-carg'):skipcond({
['Test requires JIT enabled'] = not jit.status(),
})

-- Loop unrolls into 2 iterations. Thus means that the loop is
-- executed on trace on the 5th iteration (instead of the usual
-- 4th). Run it even number of iterations to test both, so last is
-- 6th.
local NTESTS = 6

test:plan(NTESTS)

-- XXX: Hack with function's prototypes to avoid creation of
-- custom functions to be loaded via FFI (vararg part will be just
-- ignored).
ffi.cdef[[
double sin(double, ...);
double cos(double, ...);
]]

local EXPECTED = {[0] = ffi.C.sin(0), ffi.C.cos(0)}

-- Array of 2 functions.
local fns = ffi.new('double (*[2])(double, ...)')
fns[0] = ffi.C.cos
fns[1] = ffi.C.sin

-- Avoid reallocating the table on the trace.
local result = table_new(8, 0)

jit.opt.start('hotloop=1')

local fn = fns[0]
-- The first result is `cos()`.
for i = 1, NTESTS do
result[i] = fn(0)
fn = fns[i % 2]
-- The call persists in the invariant part of the loop as well.
-- Hence, XLOAD (part of the IR_CARG -- function to be called)
-- should be marked as PHI, but it isn't due to CSE.
fn(0)
end

for i = 1, NTESTS do
test:is(result[i], EXPECTED[i % 2],
('correct result on iteration %d'):format(i))
end

test:done(true)

0 comments on commit b52fe97

Please sign in to comment.