Behavior of overflowing floating-point to integer conversions

[dzaima]

This issue is either:

a) a missed optimization in the x86-64 backend; or
b) a wrong optimization in the aarch64 backend.

Consider the following code:

#include<stdint.h>
#include<stdbool.h>
bool is_i8(double x) {
    return x == (int8_t) x;
}
bool is_i32(double x) {
    return x == (int32_t)x;
}

As per the C standard, these functions invoke undefined behavior if given arguments that, rounded to an integer, don't fit in the desired type. Thus, is_i8 can always be replaced with is_i32 (alive2 proof from the optimized IR).

However, the x86-64 backend does not do this, and thus is_i8 has an unnecessary movsx eax, al instruction that can be eliminated. The aarch64 backend does do this optimization. compiler explorer.

GCC preserves the int8_t cast on both x86-64 and ARM64 (and everything else I tested in CE): https://godbolt.org/z/78MbdGbPz. (EDIT: on gcc≤13.2, this is only the case with -ftrapping-math, which gcc has on by default - adding -fno-trapping-math (and -msse4.1 on x86-64) it'll convert it to a round-comparison; on gcc≥13.3 it keeps the integer conversion always as far as I can tell)

And while C does allow the optimization in question, it means that x == (T)x cannot be used as a check for whether the floating-point value x fits in the integer type T (even though it would work if the conversion gave any valid value of T in place of UB/poison). And, as far as I can tell, there is no alternative way to do a check like this anywhere near as performantly, without writing platform-specific assembly, which is, IMO, a quite problematic issue, though not really a clang-specific one.

[dzaima]

Some additional info - in the C standard, Annex F, F.4, says that, on float to integer conversion of an overflowing value (NaN, infinity, exceeded range), "the resulting value is unspecified.".

Granted, LLVM does not, to my understanding, claim to support Annex F, but this is a nudge towards making the result unspecified instead of undefined (which would mean that x == (int8_t) x would be usable as a fits-in-int8 check, and would match option B in the original issue).

[nikic]

Clang considers this undefined behavior by default, but you can use -fno-strict-float-cast-overflow to make overflowing float to int casts well-defined. (The documentation does not mention it, but they become saturating.)

[dzaima]

Thanks, that's a useful note. Though unfortunately it saturating means significantly more generated code on x86-64 & risc-v, so it's not much better in my case where I do want it to be as fast as possible.

I suppose, with, -fno-strict-float-cast-overflow, x == (int8_t)x could be optimized to not do the saturation, but it'd still slow down other things doing a cast.

[nikic]

As we don't seem to promise any specific behavior in the documentation, it might be possible to change it use freeze(fptoui) instead of fptui.sat, which would be about what you want. Not sure whether people rely on the current saturating implementation.

[dzaima]

Seems adding a freeze over fptosi in my as_i8 example doesn't actually change the behavior of the aarch64 & risc-v output: https://godbolt.org/z/YznfaEsnb; I guess that's a miscompilation?

[nikic]

Simpler example: https://godbolt.org/z/afeaEf8oo

This does look like a miscompile to me. We have this type-legalized SDAG:

SelectionDAG has 11 nodes:
  t0: ch,glue = EntryToken
            t2: f64,ch = CopyFromReg t0, Register:f64 %0
          t9: i32 = fp_to_sint t2
        t11: i32 = AssertSext t9, ValueType:ch:i8
      t12: i32 = freeze t11
    t13: i32 = sign_extend_inreg t12, ValueType:ch:i8
  t7: ch,glue = CopyToReg t0, Register:i32 $w0, t13
  t8: ch = AArch64ISD::RET_GLUE t7, Register:i32 $w0, t7:1

Which gets DAGCombined to:

  t0: ch,glue = EntryToken
          t2: f64,ch = CopyFromReg t0, Register:f64 %0
        t9: i32 = fp_to_sint t2
      t14: i32 = freeze t9
    t11: i32 = AssertSext t14, ValueType:ch:i8
  t7: ch,glue = CopyToReg t0, Register:i32 $w0, t11
  t8: ch = AArch64ISD::RET_GLUE t7, Register:i32 $w0, t7:1

Note that the freeze was moved past the AssertSext. This happens because these are explicitly listed as not creation undef/poison:

llvm-project/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

Lines 5006 to 5007 in 3f4bf99

	case ISD::AssertSext:
	case ISD::AssertZext:

This looks incorrect to me.

[nikic]

This was introduced in 7e294e676e32f by @RKSimon. What was the rationale for that change? Wouldn't these nodes return poison if the assertion does not hold?

[RKSimon]

IIRC the descriptions at the moment doesn't allow for cases where the assert nodes don't hold.

[RKSimon]

Feel free to remove the assertzext/sext cases if it's causing a problem and we can revisit this.