Skip to content
This repository has been archived by the owner. It is now read-only.

asm_conv: Assertion `((IRType)((ir->t).irt & IRT_TYPE)) != st' failed. #37

Closed
spacewander opened this issue Nov 20, 2019 · 5 comments · Fixed by #39
Closed

asm_conv: Assertion `((IRType)((ir->t).irt & IRT_TYPE)) != st' failed. #37

spacewander opened this issue Nov 20, 2019 · 5 comments · Fixed by #39

Comments

@spacewander
Copy link

Hi, I have seen your email in the LuaJIT mail list. It is a good news that you are pushing LuaJIT ahead.

Several days ago, I submitted a bug report to the LuaJIT but no response is given back.
LuaJIT#524

The bug can be reproduced with the master branch of moonjit, so I think you might feel interested about it.

I have reduced the reproduce script to a few lines:

local bit = require "bit"
local ffi = require "ffi"

ffi.cdef[[
    typedef unsigned int u32;
]]
local prime = 16777619

local function prepare_hash(s)
    -- change this line to `sq = prime` eliminates the crash
    local sq = ffi.cast("u32", prime)
    local i = #s
    while i > 0 do
        -- crash when LuaJIT try to compile this while block
        sq = ffi.cast("u32", sq * sq)
        i = bit.rshift(i, 1)
    end
end

for case = 1, 1000 do
    prepare_hash("12345678")
end

When the lua_assert(irt_type(ir->t) != st) failed, irt_type(ir->t) is IRT_INT.
So maybe something wrong happens when LuaJIT generates an invalid IR CONV INT to INT, triggers assertion failure during converting IR to mcode.

@fsfod
Copy link

fsfod commented Nov 22, 2019

It looks like simplify_conv_narrow is always emitting a signed CONV even if the type its folding is U32

moonjit/src/lj_opt_fold.c

Lines 1258 to 1272 in 3f39980

LJFOLDF(simplify_conv_narrow)
{
IROp op = (IROp)fleft->o;
IRType t = irt_type(fins->t);
IRRef op1 = fleft->op1, op2 = fleft->op2, mode = fins->op2;
PHIBARRIER(fleft);
op1 = emitir(IRTI(IR_CONV), op1, mode);
op2 = emitir(IRTI(IR_CONV), op2, mode);
fins->ot = IRT(op, t);
fins->op1 = op1;
fins->op2 = op2;
return RETRYFOLD;
}
/* Special CSE rule for CONV. */

siddhesh added a commit that referenced this issue Nov 23, 2019
Fixes #37.

simplify_conv_narrow transforms a 64-bit to 32-bit int conversion
after an operation (e.g. add, sub, mul) to an op of the converted
operands by simply converting the operands into 32-bit int.  It
however fails to update the destination type of the conversion mode
because of which one ends up with inconsistent IR and fails with an
assertion.

The test case is incorporated into this patch but it doesn't actually
get executed in the make check.  ffi_convert.lua needs extensive
fixing up to enable it in the testsuite and is currently out of scope
for 2.1.2.

Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
Signed-off-by: spacewander <spacewanderlzx@gmail.com>
siddhesh added a commit that referenced this issue Nov 23, 2019
Fixes #37.

simplify_conv_narrow transforms a 64-bit to 32-bit int conversion
after an operation (e.g. add, sub, mul) to an op of the converted
operands by simply converting the operands into 32-bit int.  It
however fails to update the destination type of the conversion mode
because of which one ends up with inconsistent IR and fails with an
assertion.

The test case is incorporated into this patch but it doesn't actually
get executed in the make check.  ffi_convert.lua needs extensive
fixing up to enable it in the testsuite and is currently out of scope
for 2.1.2.

Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
Signed-off-by: spacewander <spacewanderlzx@gmail.com>
@siddhesh
Copy link
Collaborator

It looks like simplify_conv_narrow is always emitting a signed CONV even if the type its folding is U32

Thanks for the tip, that helped me find the problem immediately. The INT assumption is fine, just that the conversion mode is not updated to reflect that reality. I've posted (and will push soon) #39 that should fix this problem.

siddhesh added a commit that referenced this issue Nov 23, 2019
Fixes #37.

simplify_conv_narrow transforms a 64-bit to 32-bit int conversion
after an operation (e.g. add, sub, mul) to an op of the converted
operands by simply converting the operands into 32-bit int.  It
however fails to update the destination type of the conversion mode
because of which one ends up with inconsistent IR and fails with an
assertion.

The test case is incorporated into this patch but it doesn't actually
get executed in the make check.  ffi_convert.lua needs extensive
fixing up to enable it in the testsuite and is currently out of scope
for 2.1.2.

Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
Signed-off-by: spacewander <spacewanderlzx@gmail.com>
siddhesh added a commit that referenced this issue Nov 23, 2019
Fixes #37.

simplify_conv_narrow transforms a 64-bit to 32-bit int conversion
after an operation (e.g. add, sub, mul) to an op of the converted
operands by simply converting the operands into 32-bit int.  It
however fails to update the destination type of the conversion mode
because of which one ends up with inconsistent IR and fails with an
assertion.

The test case is incorporated into this patch but it doesn't actually
get executed in the make check.  ffi_convert.lua needs extensive
fixing up to enable it in the testsuite and is currently out of scope
for 2.1.2.

Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
Signed-off-by: spacewander <spacewanderlzx@gmail.com>
@siddhesh
Copy link
Collaborator

Fixed in master and v2.1 branch, so 2.1.2 should have this.

@fsfod
Copy link

fsfod commented Nov 23, 2019

Shouldn't the signedness of the IR op be preserved?

@siddhesh
Copy link
Collaborator

It doesn't matter for integer ADD, SUB or MUL since they don't care about signedness.

There is one tiny issue with this approach: coercing everything to INT has the side effect that simplify_conv_int_i64 will fail to see conversions of U32 -> U64 -> U32 and optimise them out in such situations since the last U64->U32 gets coerced to U64->INT in simplify_narrow_conv. So it's a missed optimization in the interest of very slightly faster compilation.

That said, in this case the conversion sequence is INT->I64->U32 and here the INT coercion helps eliminate the conversion. So this specific test case is faster. I'll open a separate issue for this.

siddhesh pushed a commit that referenced this issue Nov 26, 2019
Alternative fix for #37

When emitting CONV make sure that its type matches its destination IRType.

This keeps IR fully internally consistent with respect to types - i.e. if
we push narrowing CONV Dt.St upwards through an arithmetic operation of type
St we end up with arithmetic operation of type Dt and two convertions
CONV Dt.St which narrow the operands.

Previous variantion of the fix introduced slight inconsistency with types
(inserted convertions were CONV int.St while arithmetic operation was still of
type Dt).

Signed-off-by: Vyacheslav Egorov <vegorov@google.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants