[Tarantool-patches] [PATCH luajit] x86/x64: Don't use undefined MUL/IMUL zero flag.

[Tarantool-patches] [PATCH luajit] x86/x64: Don't use undefined MUL/IMUL zero flag.

[Sergey Kaplun via Tarantool-patches]

From: Mike Pall <mike>

Reported by VrIgHtEr.

(cherry picked from commit c92d0cb19263e7e302b4740ba6617a32c201c613)

When emitting the arithmetic operations on registers via the
`asm_intarith()`, the next `test` instruction may be dropped since the
flag register is modified by the arithmetic instruction to be emitted.

But the `imul` instruction [1] doesn't modify ZF, so its value is
undefined. This patch prevents dropping the `test` instruction if
the emitted instruction is `imul`.

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

[1]: https://www.felixcloutier.com/x86/imul

Part of tarantool/tarantool#11691
---

Branch: https://github.com/tarantool/luajit/tree/skaplun/lj-1376-undefined-mul-test-flag
Related issues:
* https://github.com/tarantool/tarantool/issues/11691
* https://github.com/LuaJIT/LuaJIT/issues/1376

 src/lj_asm_x86.h                              |  3 +-
 .../lj-1376-undefined-mul-test-flag.test.lua  | 38 +++++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)
 create mode 100644 test/tarantool-tests/lj-1376-undefined-mul-test-flag.test.lua

diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h
index 89e83205..0e3a473c 100644
--- a/src/lj_asm_x86.h
+++ b/src/lj_asm_x86.h
@@ -2061,7 +2061,8 @@ static void asm_intarith(ASMState *as, IRIns *ir, x86Arith xa)
   RegSet allow = RSET_GPR;
   Reg dest, right;
   int32_t k = 0;
-  if (as->flagmcp == as->mcp) {  /* Drop test r,r instruction. */
+  if (as->flagmcp == as->mcp && xa != XOg_X_IMUL) {
+    /* Drop test r,r instruction. */
     MCode *p = as->mcp + ((LJ_64 && *as->mcp < XI_TESTb) ? 3 : 2);
     MCode *q = p[0] == 0x0f ? p+1 : p;
     if ((*q & 15) < 14) {
diff --git a/test/tarantool-tests/lj-1376-undefined-mul-test-flag.test.lua b/test/tarantool-tests/lj-1376-undefined-mul-test-flag.test.lua
new file mode 100644
index 00000000..f6c02a00
--- /dev/null
+++ b/test/tarantool-tests/lj-1376-undefined-mul-test-flag.test.lua
@@ -0,0 +1,38 @@
+local tap = require('tap')
+
+-- Test file to demonstrate incorrect assembling optimization
+-- for x86/x64 CPUs.
+-- See also: https://github.com/LuaJIT/LuaJIT/issues/1376.
+
+local test = tap.test('lj-1376-undefined-mul-test-flag'):skipcond({
+  ['Test requires JIT enabled'] = not jit.status(),
+})
+
+test:plan(1)
+
+local a, b = 0ULL, 0ULL
+
+jit.opt.start('hotloop=1')
+for _ = 1, 4 do
+  -- Before the patch, the `test` instruction is dropped by
+  -- assuming the `imul` instruction before it modifies the flags
+  -- register. It results in the following mcode:
+  -- | imul r15, rbp
+  -- | jnz 0x559415b10060        ->5
+  -- Instead of the following:
+  -- | imul r15, rbp
+  -- | test r15, r15
+  -- | jnz 0x559415b10060        ->5
+  -- This leads to the incorrect branch being taken.
+  if a * b ~= 0ULL then
+    test:fail('the impossible branch is taken')
+    test:done(true)
+  end
+  -- XXX: Need to update multiplier to stay in the variant part of
+  -- the loop, since invariant contains IR_NOP (former unused
+  -- IR_CNEW) between IRs, and the optimization is not applied.
+  b = b + 1
+end
+
+test:ok(true, 'no dropping of test instruction')
+test:done(true)
-- 
2.50.1

Re: [Tarantool-patches] [PATCH luajit] x86/x64: Don't use undefined MUL/IMUL zero flag.

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 3405 bytes --]

Hi,  Sergey,

thanks for the patch! LGTM

Sergey

On 8/20/25 14:49, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by VrIgHtEr.
>
> (cherry picked from commit c92d0cb19263e7e302b4740ba6617a32c201c613)
>
> When emitting the arithmetic operations on registers via the
> `asm_intarith()`, the next `test` instruction may be dropped since the
> flag register is modified by the arithmetic instruction to be emitted.
>
> But the `imul` instruction [1] doesn't modify ZF, so its value is
> undefined. This patch prevents dropping the `test` instruction if
> the emitted instruction is `imul`.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> [1]:https://www.felixcloutier.com/x86/imul
>
> Part of tarantool/tarantool#11691
> ---
>
> Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1376-undefined-mul-test-flag
> Related issues:
> *https://github.com/tarantool/tarantool/issues/11691
> *https://github.com/LuaJIT/LuaJIT/issues/1376
>
>   src/lj_asm_x86.h                              |  3 +-
>   .../lj-1376-undefined-mul-test-flag.test.lua  | 38 +++++++++++++++++++
>   2 files changed, 40 insertions(+), 1 deletion(-)
>   create mode 100644 test/tarantool-tests/lj-1376-undefined-mul-test-flag.test.lua
>
> diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h
> index 89e83205..0e3a473c 100644
> --- a/src/lj_asm_x86.h
> +++ b/src/lj_asm_x86.h
> @@ -2061,7 +2061,8 @@ static void asm_intarith(ASMState *as, IRIns *ir, x86Arith xa)
>     RegSet allow = RSET_GPR;
>     Reg dest, right;
>     int32_t k = 0;
> -  if (as->flagmcp == as->mcp) {  /* Drop test r,r instruction. */
> +  if (as->flagmcp == as->mcp && xa != XOg_X_IMUL) {
> +    /* Drop test r,r instruction. */
>       MCode *p = as->mcp + ((LJ_64 && *as->mcp < XI_TESTb) ? 3 : 2);
>       MCode *q = p[0] == 0x0f ? p+1 : p;
>       if ((*q & 15) < 14) {
> diff --git a/test/tarantool-tests/lj-1376-undefined-mul-test-flag.test.lua b/test/tarantool-tests/lj-1376-undefined-mul-test-flag.test.lua
> new file mode 100644
> index 00000000..f6c02a00
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1376-undefined-mul-test-flag.test.lua
> @@ -0,0 +1,38 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate incorrect assembling optimization
> +-- for x86/x64 CPUs.
> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1376.
> +
> +local test = tap.test('lj-1376-undefined-mul-test-flag'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +local a, b = 0ULL, 0ULL
> +
> +jit.opt.start('hotloop=1')
> +for _ = 1, 4 do
> +  -- Before the patch, the `test` instruction is dropped by
> +  -- assuming the `imul` instruction before it modifies the flags
> +  -- register. It results in the following mcode:
> +  -- | imul r15, rbp
> +  -- | jnz 0x559415b10060        ->5
> +  -- Instead of the following:
> +  -- | imul r15, rbp
> +  -- | test r15, r15
> +  -- | jnz 0x559415b10060        ->5
> +  -- This leads to the incorrect branch being taken.
> +  if a * b ~= 0ULL then
> +test:fail('the impossible branch is taken')
> +test:done(true)
> +  end
> +  -- XXX: Need to update multiplier to stay in the variant part of
> +  -- the loop, since invariant contains IR_NOP (former unused
> +  -- IR_CNEW) between IRs, and the optimization is not applied.
> +  b = b + 1
> +end
> +
> +test:ok(true, 'no dropping of test instruction')
> +test:done(true)

[-- Attachment #2: Type: text/html, Size: 4367 bytes --]

Re: [Tarantool-patches] [PATCH luajit] x86/x64: Don't use undefined MUL/IMUL zero flag.

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 3403 bytes --]

Hi, Sergey,

thanks for the patch! LGTM

Sergey

On 8/20/25 14:49, Sergey Kaplun wrote:
> From: Mike Pall <mike>
>
> Reported by VrIgHtEr.
>
> (cherry picked from commit c92d0cb19263e7e302b4740ba6617a32c201c613)
>
> When emitting the arithmetic operations on registers via the
> `asm_intarith()`, the next `test` instruction may be dropped since the
> flag register is modified by the arithmetic instruction to be emitted.
>
> But the `imul` instruction [1] doesn't modify ZF, so its value is
> undefined. This patch prevents dropping the `test` instruction if
> the emitted instruction is `imul`.
>
> Sergey Kaplun:
> * added the description and the test for the problem
>
> [1]:https://www.felixcloutier.com/x86/imul
>
> Part of tarantool/tarantool#11691
> ---
>
> Branch:https://github.com/tarantool/luajit/tree/skaplun/lj-1376-undefined-mul-test-flag
> Related issues:
> *https://github.com/tarantool/tarantool/issues/11691
> *https://github.com/LuaJIT/LuaJIT/issues/1376
>
>   src/lj_asm_x86.h                              |  3 +-
>   .../lj-1376-undefined-mul-test-flag.test.lua  | 38 +++++++++++++++++++
>   2 files changed, 40 insertions(+), 1 deletion(-)
>   create mode 100644 test/tarantool-tests/lj-1376-undefined-mul-test-flag.test.lua
>
> diff --git a/src/lj_asm_x86.h b/src/lj_asm_x86.h
> index 89e83205..0e3a473c 100644
> --- a/src/lj_asm_x86.h
> +++ b/src/lj_asm_x86.h
> @@ -2061,7 +2061,8 @@ static void asm_intarith(ASMState *as, IRIns *ir, x86Arith xa)
>     RegSet allow = RSET_GPR;
>     Reg dest, right;
>     int32_t k = 0;
> -  if (as->flagmcp == as->mcp) {  /* Drop test r,r instruction. */
> +  if (as->flagmcp == as->mcp && xa != XOg_X_IMUL) {
> +    /* Drop test r,r instruction. */
>       MCode *p = as->mcp + ((LJ_64 && *as->mcp < XI_TESTb) ? 3 : 2);
>       MCode *q = p[0] == 0x0f ? p+1 : p;
>       if ((*q & 15) < 14) {
> diff --git a/test/tarantool-tests/lj-1376-undefined-mul-test-flag.test.lua b/test/tarantool-tests/lj-1376-undefined-mul-test-flag.test.lua
> new file mode 100644
> index 00000000..f6c02a00
> --- /dev/null
> +++ b/test/tarantool-tests/lj-1376-undefined-mul-test-flag.test.lua
> @@ -0,0 +1,38 @@
> +local tap = require('tap')
> +
> +-- Test file to demonstrate incorrect assembling optimization
> +-- for x86/x64 CPUs.
> +-- See also:https://github.com/LuaJIT/LuaJIT/issues/1376.
> +
> +local test = tap.test('lj-1376-undefined-mul-test-flag'):skipcond({
> +  ['Test requires JIT enabled'] = not jit.status(),
> +})
> +
> +test:plan(1)
> +
> +local a, b = 0ULL, 0ULL
> +
> +jit.opt.start('hotloop=1')
> +for _ = 1, 4 do
> +  -- Before the patch, the `test` instruction is dropped by
> +  -- assuming the `imul` instruction before it modifies the flags
> +  -- register. It results in the following mcode:
> +  -- | imul r15, rbp
> +  -- | jnz 0x559415b10060        ->5
> +  -- Instead of the following:
> +  -- | imul r15, rbp
> +  -- | test r15, r15
> +  -- | jnz 0x559415b10060        ->5
> +  -- This leads to the incorrect branch being taken.
> +  if a * b ~= 0ULL then
> +test:fail('the impossible branch is taken')
> +test:done(true)
> +  end
> +  -- XXX: Need to update multiplier to stay in the variant part of
> +  -- the loop, since invariant contains IR_NOP (former unused
> +  -- IR_CNEW) between IRs, and the optimization is not applied.
> +  b = b + 1
> +end
> +
> +test:ok(true, 'no dropping of test instruction')
> +test:done(true)

[-- Attachment #2: Type: text/html, Size: 4365 bytes --]