Fix x86-64 JIT conversion of negative floats to unsigned ints

[wingo]

This patch fixes a problem where the JITted code for float-to-unsigned-int conversions was not matching the behavior of the interpreted code.

The basic problem is that the domain of cvttsd2si doesn't match the domain of uint64_t or uint32_t.

The previous code would assume a negative result indicated an error and would retry with the input minus 2^64. However this isn't what we should do for an input of -1, for example; although uint64 u = -1.0 is undefined, strictly speaking, as it's an out-of-range conversion, common LuaJIT and C practice means we should accept the result to be 0xffffffffffffffff. The interpreter inherits this behavior from the C compiler.

This patch changes the JIT to match the interpreter. In the future we should consider making the interpreter's result more explicit, e.g. by changing the implementation of lj_num2u64 to be

uint64_t
lj_num2u64 (double n)
{
  if (n < -2^63) then return INT_MIN;
  if (n < 2^63) then return n;
  if (n < 2^64) then return n - 2^64;
  return INT_MIN;
}

Thanks to Peter Cawley (@corsix) for advice, help on the test case, an initial patch, and suggested assembly, which looks like:

2a50ffb3  cvttsd2si rbx, xmm7
2a50ffb8  movaps xmm6, xmm7
2a50ffbb  subsd xmm6, [0x41c52680] ;; 2^64
2a50ffc4  cvttsd2si rdi, xmm6
2a50ffc9  cmp rbx, +0x01
2a50ffcd  cmovo rbx, rdi

Corresponding changes were made to the conversion from 32-bit floats and conversion to 32-bit unsigned integers.

See justincormack/ljsyscall#223 for the original discussion.

[wingo]

Forgot to attach the test case:

-- Thanks to Peter Cawley for these tests.

local cast = require"ffi".cast
local tonumber = tonumber
local t = {}

-- (1): Check that you can cast negative floating-point numbers to
-- unsigned integers.
for i = 1, 70 do
  t[i] = tonumber(cast("int64_t", cast("uint64_t", -(i+0.2))))
end
for i = 1, 70 do
  -- Technically undefined, but seems to match gcc/clang behaviour.
  assert(t[i] == -i)
end

-- (2) Check that you can cast floating-point numbers from the upper
-- half of the unsigned integer range to unsigned integers.
for i = 1, 70 do
  t[i] = (tonumber(cast("uint64_t", (2^63 + 2^50 * i))) - 2^63) / 2^50
end
for i = 1, 70 do
  assert(t[i] == i)
end

-- (3): Same as (1), but for uint32_t.
for i = 1, 70 do
  t[i] = tonumber(cast("int32_t", cast("uint32_t", -(i+0.2))))
end
for i = 1, 70 do
  assert(t[i] == -i)
end

-- (4): Same as (2), but for uint32_t.
for i = 1, 70 do
  t[i] = (tonumber(cast("uint32_t", (2^31 + 2^18 * i))) - 2^31) / 2^18
end
for i = 1, 70 do
  assert(t[i] == i)
end

[MikePall]

Replicating undefined behavior of any particular compiler on a particular platform is an explicit non-goal. Especially, as this sends a false signal to developers.

What you refer to "common practice" is an unwarranted and unportable assumption. See C99 or C11 6.3.1.4, footnote of (1).

The assumption is invalid e.g. on ARM64, as this has FCVTZU Xd, Dn, which both C compilers and LuaJIT happily make use of. That particular instruction is specified to return zero for out-of-range inputs. Of course, that's just another outcome of undefined behavior on a particular platform.

Anyway, consistency between the interpreter and compiler is still desirable. So I've made the interpreter return the same results as the JIT compiler backend. Which likely helps to detect the assumption as invalid, right away.

--

EDIT: Misread the ARM64 specs. Corrected out-of-range result. But this is tangential to the discussion.

[javierguerragiraldez]

just tried it and found that now 0ull-1 ~= 0ull+-1

i fail to see this as an improvement.

[MikePall]

The second expression invokes undefined behavior. You're just getting a different flavor than before.

Presumably, you want to write portable code. E.g. it looks like your employer is investing into ARM64 servers, if I understood the official blogs right. And you probably want to get identical results from your code across platforms. That's why you should strive to avoid undefined behavior.

OK, so let's try your code on ARM64 (on real hardware, not QEMU!). But wait ... there's dual-number mode, so -1 is really an integer. And the interpreter constant-folds, too. So let's trick it into using a double for the implicit conversion:

Before the change:

x64$ luajit-260b9b48 -e 'local x=0.5; local y=x-1.5; print(0ull+y)'
18446744073709551615ULL

arm64$ luajit-260b9b48 -e 'local x=0.5; local y=x-1.5; print(0ull+y)'
0ULL

After the change:

x64$ luajit-v2.1 -e 'local x=0.5; local y=x-1.5; print(0ull+y)'
9223372036854775808ULL

arm64$ luajit-v2.1 -e 'local x=0.5; local y=x-1.5; print(0ull+y)'
0ULL

Surprised?

Per the FFI semantics, a Lua number is converted to the FFI type, i.e. uint64_t, before a mixed-type arithmetic operation. That's of course an undefined conversion for a negative double, so it's unsurprising to get different results on different platforms.

Also, please note I'm not too concerned about the speed of the interpreter in this particular case (the interpreter is rather slow for FFI operations, anyway). We can use whatever code works best -- as long as it matches the JIT backend behavior.

But the JIT compiler turns such a conversion into a single ARM64 instruction or a couple of x64 instructions (lacking a fitting native instruction). It would obviously be helpful for performance to be able to use that single instruction.

So, what would you like me to do? Slow down ARM64 (and most other platforms) to make some undefined behavior defined?

[I already explained the expectations about the double -> uint64_t conversion behavior in C are unwarranted and based on the observation of some platform-dependent behavior. That C code would give different results on ARM64, too.]

[javierguerragiraldez]

yes, undefined cases have undefined results, no matter what "common sense" would prefer; i agree on that.

but i'm seeing even less consistency now, especially between the interpreted and compiled code, on the same platform (Linux, amd64):

local t = {}

for i=1,100 do
	t[i] = 0ull + -1
end

for i = 50,70 do
	print(i, bit.tohex(t[i]))
end

used to give:

50      ffffffffffffffff
51      ffffffffffffffff
52      ffffffffffffffff
53      ffffffffffffffff
54      ffffffffffffffff
55      ffffffffffffffff
56      ffffffffffffffff
57      ffffffffffffffff
58      ffffffffffffffff
59      ffffffffffffffff
60      ffffffffffffffff
61      ffffffffffffffff
62      ffffffffffffffff
63      ffffffffffffffff
64      ffffffffffffffff
65      ffffffffffffffff
66      ffffffffffffffff
67      ffffffffffffffff
68      ffffffffffffffff
69      ffffffffffffffff
70      ffffffffffffffff

but after the patch:

50      8000000000000000
51      8000000000000000
52      8000000000000000
53      8000000000000000
54      8000000000000000
55      8000000000000000
56      8000000000000000
57      8000000000000000
58      ffffffffffffffff
59      ffffffffffffffff
60      ffffffffffffffff
61      ffffffffffffffff
62      ffffffffffffffff
63      ffffffffffffffff
64      ffffffffffffffff
65      8000000000000000
66      ffffffffffffffff
67      ffffffffffffffff
68      ffffffffffffffff
69      ffffffffffffffff
70      ffffffffffffffff

[MikePall]

The JIT compiler is too clever for its own good. It narrows the double (-1) to an integer, when it's able to guarantee the operation (+) result is identical ... that is, if the operation is defined. Which it is not, of course.

OK, so this is not workable. Alternative, consistent solutions, anyone? For all architectures, of course. Preferably with attached benchmarks on their performance impact.

[nico-abram]

Should an issue about this be opened for discussion?

[siddhesh]

There should be a way to do this in two stages that is still conforming: convert from negative float to signed integer (which is defined as long as the truncated number is within the range of the signed integer type) and then cast the signed integer to unsigned.

[siddhesh]

FWIW, this is what I'm proposing:

moonjit@454bea8

This ends up with an additional branch but it should have an insignificant performance impact for most out of order processors.

[corsix]

Alternative, consistent solutions, anyone?

As background, there are the C-style conversions float/double -> int32/uint32/int64/uint64, all of which are defined as:

1. FP domain round toward zero (i.e. truncation). [NB: no-op for operands with abs(x) >= 2⁵²]
2. Convert to infinite-precision integer.
3. Undefined behaviour if the infinite-precision integer is outside the range of the target type (that being one of int32/uint32/int64/uint64).
4. Return the value in the target type equal to the infinite-precision integer.

At the moment, FFI-style conversions match the C-style conversions. I've been considering changing the FFI-style conversions to be just two core conversions; one double -> 32-bit (N=32) and one double -> 64-bit (N=64), defined as:

1. FP domain round toward zero (i.e. truncation). [NB: no-op for operands with abs(x) >= 2⁵²]
2. Convert to infinite-precision integer.
3. Undefined behaviour if the infinite-precision integer is outside the range of -2^N-1 through 2^N-1.
4. Return the value in the target type equal to the infinite-precision integer modulo 2^N.

Notably, FFI-style double -> 32-bit would then be a superset of both C-style double -> int32 and C-style double -> uint32_t, and FFI-style double -> 64-bit would then be a superset of both C-style double -> int64 and C-style double -> uint64_t. FFI-style float -> T conceptually does a float -> double promotion, followed by double -> T, though in practice there might be a more efficient direct route.

There's a soft-float implementation of FFI-style double -> 64-bit with defined behaviour for all finite inputs:

static uint64_t dbl_to_64(const double* x) {
  uint64_t m, s;
  int32_t e;
  memcpy(&m, x, 8);
  e = ((m >> 52) & 0x7ff) - (1023+52);
  if (e <= -53 || e >= 64) return 0;
  s = (uint64_t)(((int64_t)m) >> 63);
  m = (m & U64x(fffff, ffffffff)) | U64x(100000, 00000000);
  return ((e < 0 ? m >> -e : m << e) + s) ^ s;
} /* NB: If caller wants int64_t, cast the result to int64_t. */

Backends could always choose to use the soft-float implementation for double -> 64-bit, though in practice backends with an FPU available will have a more efficient option available. FFI-style double -> 32-bit could be implemented as double -> 64-bit (via soft-float if necessary), followed by an integer operation to truncate 64 bits down to 32, though in practice this is rarely the most efficient route.

If there's a hardware instruction available for C-style double -> int64 (as is the case for all 64-bit backends), then FFI-style double -> 32-bit has a simple implementation as C-style double -> int64 (e.g. x86 cvttsd2si) followed by integer operation to truncate 64 bits down to 32, with the defined range in this case being -2⁶³ through 2⁶³-1. If there's no cheap way of doing C-style double -> int64, an alternative option for FFI-style double -> 32-bit is FP domain round toward zero (e.g. SSE4.1 roundsd, ARM64 frintz), followed by bit.tobit-style double -> int32 (i.e. add 2⁵²+2⁵¹ in the FP domain, then bitwise reinterpret as integer), with the defined range in this case being +/-2⁵¹. On sufficiently modern ARM64, FFI-style double -> 32-bit could also be a single fjcvtzs instruction, with defined range being all finite values. On x86 32-bit, x87 presents a C-style double -> int64 (albeit via memory), which might or might not be preferable to the bit.tobit route.

Efficient FFI-style double -> 64-bit is trickier. On ARM64, one starting point is fcvtzu, which does an FP domain truncation followed by a saturating conversion double -> uint64_t. From this, a four-instruction sequence can be built that covers the domain -2⁶⁴-1 through 2⁶⁴-1:

fcvtzu idst, fsrc
fneg ftmp, fsrc
fcvtzu itmp, ftmp
sub idst, idst, itmp

On x64, cvttsd2si (C-style double -> int64) is the obvious building-block, but it covers 2^-63 through 2⁶³-1, and yields 2^-63 for anything out of range. As in the opening description for this ticket, it can be extended to 2^-63 through 2⁶⁴+2⁶³-1 by doing a 2^nd conversion of x-2⁶⁴ and then merging the results. The assembly from the opening description is fine, though some tweaks are also possible: using addsd with constant -2⁶⁴ rather than subsd with constant 2⁶⁴ (allowing a load of the constant followed by an addsd destroying the constant, rather than movaps of the original value followed by subsd with memory operand), and merging by summing the two results and then adding 2⁶³ rather than cmp/cmovo (possibly nicer for old CPUs with expensive cmov). x86 32-bit can take the soft-float route for FFI-style double -> 64-bit, or try the x87 alternative to cvttsd2si(fisttp).

Rough thoughts for other backends:

mips32r1:
  just use soft-float?
mips32r2+:
  to 32-bit: TRUNC.L.D, move low 32 bits to GPR
  to 64-bit: if (x >= 2^63) { x += -2^64 } TRUNC.L.D, move to GPR pair
mips64:
  to 32-bit: TRUNC.L.D, move low 32 bits to GPR
  to 64-bit: if (x >= 2^63) { x += -2^64 } TRUNC.L.D, move to GPR (i.e. existing logic for FP to U64 conversions)

arm32:
  to 32-bit: VCVT_U32_F64, VNEG, VCVT_U32_F64, SUB (or VJCVT on sufficiently modern cores)
  to 64-bit: soft-float

ppc with fctidz:
  to 32-bit: fctidz, move low 32 bits to GPR
  to 64-bit: if (x >= 2^63) { x += -2^64 } fctidz, move to GPR
ppc with fctiwz:
  to 32-bit: if (x >= 2^31) { x += -2^31; fctiwz; integer += 2^31 } else fctiwz
  to 64-bit: soft-float

riscv64:
  same strategy as arm64, just different instruction names

[Buristan]

As an alternative, we can just raise an error like the following: cannot convert 'number' to 'uint64_t', value range exceeded in cases when the given value is outside the range of the target type. Since nobody should rely on undefined (or implementation-defined) behaviour, this will signal users that there is an error in their code and they should fix it. Now there are already cases when some (arithmetic) operation can raise an error when conversion between types is invalid:

src/luajit -e '
local ffi = require"ffi"
local a = ffi.new("double", 1.1)
local b = ffi.new("void *", 0)
print(a + b)
'
src/luajit: (command line):4: cannot convert 'number' to 'void *'
stack traceback:
        [C]: in function 'new'
        (command line):4: in main chunk
        [C]: at 0x55a98f1df044

Also, it should be easily implemented for the JIT part via additional guard checks (when we fallback to the interpreter, it throws the error).

Nevertheless, I have no objections regarding Peter's approach.