Use ARM intrinsic for compatible flt-to-int

[chriskillpack]

For int conversion of floating point values larger than can fit in an int, ARM handles the situation differently from x86. This difference causes test failures. On supported ARM platforms we use an intrinsic to access the specific ARM instruction that produces the same handling as x86.

[chriskillpack]

I haven't been able to test this on MSVC, can someone check that it compiles and that LineParser::CastDblToInt uses the existing static_cast< int > code.

[mungre]

Thank you for looking into this.

It still works on x86. I've tried it locally and the automated builds and tests (see below) have passed. However it still relies on UB anywhere that jcvt isn't used, and will still give the wrong answers using GCC on ARM, e.g. on a Raspberry Pi. Casting through a 64-bit int would be simpler and work everywhere. random.cpp already uses a portable 64-bit integer type, uint_least64_t, so we know that's available.

[SteveFosdick]

Does casting a float that doesn't fit into an integer ever produce a useful result with either behaviour. Wouldn't it be better to just throw an exception if the float doesn't fit into the int?

[chriskillpack]

@SteveFosdick See https://github.com/stardot/beebasm/pull/67 for context. The change was motivated to fix a failing ASSERT but I'm not 100% convinced that the test is practically useful.

@mungre I missed the int64 thing, I'm happy to experiment and replace this approach with it. Hey I got to learn about ARM intrinsics and refresh my memory on the C preprocessor. What are your thoughts about Steve's question?

[mungre]

Without this fix, even a simple EQUD &FF00FF00 won't work because &FF00FF00 won't fit into a signed int. The fix also means that when simple integer arithmetic overflows it gives the same result (mod 2^32) as BBC BASIC would.

This is a bit of a hack but with beebasm using double to represent all numbers internally it seems unavoidable, at least not without a significant redesign.

[SteveFosdick]

Referring back to the issue with assert, treating zero as false and everything else as true, I take it the issue is that clamping the values to the destination integer range, as on ARM, clamps -1 to zero and there inverts the boolean being represented which wouldn't happen in the X86 case. But isn't the answer in this case to do the assert test while the number is still a double and not convert it to an integer?

But this has surely uncovered another bug - hence this separate issue. I am assuming here that beebasm using doubles to represent numbers is because it includes some mathematical functions from BASIC that return floating point results. That presumably means where the number needs to be converted back to an integer is when it is to be included in the program being assembled as an integer, for example EQUD.

To me, at that point, it would make more sense to detect numbers that are too big and flag an error on the line being assembled and not assemble the line with a modified version of the value whether that is clamping or modulo. I think that would work easily enough when the number in its double form is positive but greater than the value that can be represented as an unsigned 32bit integer as that can always be flagged as out of range. The issue with negative numbers is that numbers in memory or a register are not inherently signed or unsigned and a directive like EQUD doesn't tell the assembler what the programmer's intention was for that number. I think the sensible thing to do here would be to allow negative numbers up to the limit of two's compliment representation. Combining the two means allowing doubles between -2147483648 and 4294967295. Once the number has been checked (while still a double), if it's in range whether you use a 64bit integer to represent it or coax the larger positive values into the right unsigned representation (even if the compiler thinks the type is signed) is a matter of choice. Perhaps the 64 bit int would be clearer.

[mungre]

Depending on how you count them there's one bug - double to integer conversions are different on ARM - or loads of bugs.

I agree that assert could just as well use double (are NaNs true or false though?) and EQUD might benefit from a range check but these aren't the only integer conversions. There are also bitwise operations, shifts, mod and div. These conversions are happening in the middle of arithmetic expressions.

My main concern is not to break existing beebasm programs. I know that there are some that do elaborate calculations at assembly time to create lookup tables. I don't know if any of them rely on beebasm's and BBC BASIC's behaviour of 32-bit wraparound for integer calculations.

It seems to me there are two separate things:

1. Fix beebasm so that it gives the same results on ARM as x86, i.e. replace all the double to int casts with a call to a conversion function. This doesn't require any change to ASSERT.
2. Maybe change ASSERT to use a double and add range checks to integer conversions.

Step 1 is necessary in itself and is a prerequisite to step 2. Step 2 might break things. It seems prudent to keep them separate.

[mungre]

This is embarrassing! I completely misunderstood how x86 double to int conversions worked. I assumed they worked mod 2^32 because several things in beebasm only work correctly if this is true. It turns out that x86 uses a sentinel value (0x80000000) for out of range conversions. The things in beebasm have always been broken.

For example, PRINT ~(&FF000000) produces &80000000. Any use of a hex constant as a bit mask with the top-bit set (except for &8000000 itself) will give nonsense results.

In a way this is good news. Any existing code that uses out-of-range numbers is getting nonsense so we don't have to worry about breaking it. Steve's solution of handling values in the range -2147483648 to 4294967295 and failing everything else looks like a win.

I've done this in PR #69. It passes the tests and I've also checked it doesn't break bogomandel or Prince of Persia.

@chriskillpack It would be great if you could try this on ARM.

[chriskillpack]

See my comments on the PR

[chriskillpack]

Closing this as the root issue was addressed in PR #69