Unordered float comparisons are emitted as normal, ordered comparisons

[folkertdev]

In other words, I've checked and ordered vs. non-ordered is not respected:

rustc_codegen_gcc/src/builder.rs

Lines 2391 to 2413 in cb499a2

	impl ToGccComp for RealPredicate {
	fn to_gcc_comparison(&self) -> ComparisonOp {
	// TODO(antoyo): check that ordered vs non-ordered is respected.
	match *self {
	RealPredicate::RealPredicateFalse => unreachable!(),
	RealPredicate::RealOEQ => ComparisonOp::Equals,
	RealPredicate::RealOGT => ComparisonOp::GreaterThan,
	RealPredicate::RealOGE => ComparisonOp::GreaterThanEquals,
	RealPredicate::RealOLT => ComparisonOp::LessThan,
	RealPredicate::RealOLE => ComparisonOp::LessThanEquals,
	RealPredicate::RealONE => ComparisonOp::NotEquals,
	RealPredicate::RealORD => unreachable!(),
	RealPredicate::RealUNO => unreachable!(),
	RealPredicate::RealUEQ => ComparisonOp::Equals,
	RealPredicate::RealUGT => ComparisonOp::GreaterThan,
	RealPredicate::RealUGE => ComparisonOp::GreaterThan,
	RealPredicate::RealULT => ComparisonOp::LessThan,
	RealPredicate::RealULE => ComparisonOp::LessThan,
	RealPredicate::RealUNE => ComparisonOp::NotEquals,
	RealPredicate::RealPredicateTrue => unreachable!(),
	}
	}
	}

That means that various operations involving NaN behave incorrectly: the unordered comparisons always return true when one of the arguments is NaN, while the ordered ones return false. The easiest way I've found to verify the behavior is the expression f64::NAN as u64, which should evaluate to 0u64, but the GCC backend in debug mode evaluates to some large constant.

When does this actually cause issues

From what I can tell, unordered comparisons, today, are used in two cases:

• RealULT: in the rustc_codegen_gcc function fptoint_sat
• RealUNE: in the lowering of != on floats (in bin_op_to_fcmp_predicate in rustc_codegen_ssa)

The behavior of != seems correct to me, so the only problematic case is fptoint_sat

So, now what?

It looks like GCC just does not have the unordered comparisons

https://github.com/gcc-mirror/gcc/blob/3880271e94b7598b4f5d98c615b7fcddddee6d4c/gcc/jit/libgccjit.h#L1263-L1282

My suggestion is to map these operators to unreachable (they are not used today besides in fptoint_sat)

RealPredicate::RealUGT => unreachable!(),
RealPredicate::RealUGE => unreachable!(),
RealPredicate::RealULT => unreachable!(),
RealPredicate::RealULE => unreachable!(),

and in fptoint_sat, use the logic currently only used for signed integers also for unsigned integers, to correct the nan case

rustc_codegen_gcc/src/builder.rs

Lines 1883 to 1892 in cb499a2

	// Step 3: NaN replacement.
	// For unsigned types, the above step already yielded int_ty::MIN == 0 if val is NaN.
	// Therefore we only need to execute this step for signed integer types.
	if signed {
	// LLVM has no isNaN predicate, so we use (val == val) instead
	let cmp = self.fcmp(RealPredicate::RealOEQ, val, val);
	self.select(cmp, s1, zero)
	} else {
	s1
	}

Alternatively, fcmp could correct the result for unordered operations

    fn fcmp(&mut self, op: RealPredicate, lhs: RValue<'gcc>, rhs: RValue<'gcc>) -> RValue<'gcc> {
        let correct_for_nan = match op {
            RealPredicate::RealPredicateFalse => unreachable!(),
            RealPredicate::RealOEQ => false,
            RealPredicate::RealOGT => false,
            RealPredicate::RealOGE => false,
            RealPredicate::RealOLT => false,
            RealPredicate::RealOLE => false,
            RealPredicate::RealONE => false,
            RealPredicate::RealORD => unreachable!(),
            RealPredicate::RealUNO => unreachable!(),
            RealPredicate::RealUEQ => false,
            RealPredicate::RealUGT => true,
            RealPredicate::RealUGE => true,
            RealPredicate::RealULT => true,
            RealPredicate::RealULE => true,
            RealPredicate::RealUNE => false,
            RealPredicate::RealPredicateTrue => unreachable!(),
        };

        let cmp = self.context.new_comparison(self.location, op.to_gcc_comparison(), lhs, rhs);

        if correct_for_nan {
            let is_nan_lhs = self.fcmp(RealPredicate::RealOEQ, lhs, lhs);
            let is_nan_rhs = self.fcmp(RealPredicate::RealOEQ, rhs, rhs);

            let is_nan = self.new_binary_op(
                self.location,
                BinaryOp::LogicalOr,
                self.cx.bool_type,
                is_nan_lhs,
                is_nan_rhs,
            );

            self.new_binary_op(self.location, BinaryOp::LogicalOr, self.cx.bool_type, is_nan, cmp)
        } else {
            cmp
        }
    }

I'd be happy to implement either of these options (or something else, if I'm missing something here)

sidenote: is there a quick way to use ./y test to run tests in a particular file in tests/run? or just all the tests in tests/? Currently std is rebuilt every time and many tests build/run, and that's not a great feedback loop.

[antoyo]

The easiest way I've found to verify the behavior is the expression f64::NAN as u64, which should evaluate to 0u64, but the GCC backend in debug mode evaluates to some large constant.

Are you sure this is actually caused by this bug and not #75?

[folkertdev]

#75 seems to just describe the same issue, namely that fptoint_sat gives incorrect results in debug mode? In release mode 0.0 is emitted as expected, so it really is the logic emitted by GCC that is incorrect here.

The logic in fptoint_sat, as ported from LLVM, relies on the semantics of RealULT, and I don't see how GCC could differentiate between the unordered and ordered version of that comparisons automatically. Hence, it can't correctly implement RealULT.

[antoyo]

#75 seems to just describe the same issue, namely that fptoint_sat gives incorrect results in debug mode?

This is possible, but as I mentioned in issue #75, I thought this was caused by the fact that GCC and clang give a different result for 0.0 / 0.0, which is how NAN is implemented in std.

Maybe I'm wrong about this, though.

[antoyo]

Also, as mentioned here:

we are not guaranteeing anything about the sign of a NaN produced by 0.0 / 0.0

So, I'm not sure what to do about this if there's no guarantee.

[folkertdev]

I don't think the sign should be relevant. This line

        let less_or_nan = self.fcmp(RealPredicate::RealULT, val, f_min);

should evaluate to true if val is any NaN, no matter the sign or which bitwise representation is picked exactly. But with an ordered compare, it will return false. I believe that is what's happening.

[antoyo]

Ok.
Please try to implement your solution in fcmp and see if this works.
Thanks for looking into this.

[folkertdev]

Ok, I'll try that. Do you have tips for faster test feedback? I think I'll add a tests/run/float.rs (to mirror tests/run/int.rs), how can I run those, and only those, tests quickly (i.e. with minimal rebuilding every time)?

[antoyo]

Those tests are not ran via ./y.sh test. You can set manually LD_LIBRARY_PATH and LIBRARY_PATH and run cargo test to run those tests.