diff --git a/src/lower_il.c b/src/lower_il.c index 34ee33cb6f..832fee1d98 100644 --- a/src/lower_il.c +++ b/src/lower_il.c @@ -16328,6 +16328,7 @@ Floating-point types require additional run-time checking for NaN (see below). an_expr_node_ptr op2 = op1->next; an_expr_node_ptr temp1, temp2, one, zero, minus_one, cmp, unordered, eptr; a_boolean op1_has_side_effects, op2_has_side_effects; + a_boolean temp1_used, temp2_used; a_type_kind type_kind = expr->variant.operation.type_kind; one = spaceship_result_constant_expr(1, expr->type); @@ -16360,15 +16361,24 @@ Floating-point types require additional run-time checking for NaN (see below). minus_one = spaceship_result_constant_expr(-1, expr->type); op1_has_side_effects = node_has_side_effects(op1, (a_boolean *)NULL); op2_has_side_effects = node_has_side_effects(op2, (a_boolean *)NULL); - temp1 = make_reusable_copy(op1, op1_has_side_effects); - temp2 = make_reusable_copy(op2, op2_has_side_effects); + temp1 = make_reusable_copy_full(op1, op1_has_side_effects, &temp1_used, + /*treat_as_potential_prvalue=*/FALSE); + temp2 = make_reusable_copy_full(op2, op2_has_side_effects, &temp2_used, + /*treat_as_potential_prvalue=*/FALSE); if (type_kind_is_simple_float_like(type_kind)) { /* If either operand is a NaN, then return - std::partial_ordering::unordered: - (__isnan(temp1 = x) || __isnan(temp2 = y)) ? RT(unordered) : + std::partial_ordering::unordered. Each operand is evaluated + exactly once, and the NaN test and the comparisons use the same + operand values: + ((temp1 = x), (temp2 = y), __isnan(temp1) | __isnan(temp2)) + ? RT(unordered) : temp1 < temp2 ? RT(-1) : temp2 < temp1 ? RT(1) : RT(0) + An operand that needs a temporary is assigned to it before the NaN + test. An operand that needs no temporary is used directly, and its + assignment to temp1 or temp2 is omitted; it is read only by the NaN + test and the comparisons. */ /* Note that the value of std::partial_ordering::unordered can vary based on the implementation. Look for its definition in the headers @@ -16386,6 +16396,22 @@ Floating-point types require additional run-time checking for NaN (see below). check_assertion(!ovflo); unordered = spaceship_result_constant_expr((int)unordered_val, expr->type); + /* Make "__isnan(temp1) | __isnan(temp2)". */ + eptr = make_isnan_call(make_reusable_copy(temp1, + /*vars_can_change=*/FALSE)); + eptr->next = make_isnan_call(make_reusable_copy(temp2, + /*vars_can_change=*/FALSE)); + eptr = make_operator_node(eok_or, integer_type(ik_int), eptr); + /* Evaluate the operands that were copied to temporaries before the + NaN test. */ + op1->next = NULL; + op2->next = NULL; + if (temp2_used) { + eptr = make_comma_node(op2, eptr); + } /* if */ + if (temp1_used) { + eptr = make_comma_node(op1, eptr); + } /* if */ temp2->next = temp1; cmp = make_operator_node(eok_lt, integer_type(ik_int), temp2); cmp->next = one; @@ -16398,11 +16424,6 @@ Floating-point types require additional run-time checking for NaN (see below). cmp = make_operator_node(eok_lt, integer_type(ik_int), temp1); cmp->next = minus_one; cmp = make_operator_node(eok_question, expr->type, cmp); - op2->next = NULL; - eptr = make_isnan_call(op2); - op1->next = NULL; - eptr->next = make_isnan_call(op1); - eptr = make_operator_node(eok_lor, integer_type(ik_int), eptr); unordered->next = cmp; eptr->next = unordered; set_node_operator(expr, eok_question, expr->type, expr->is_lvalue, eptr); diff --git a/src/pending_changes/gh-520-spaceship-nan-eval-order.txt b/src/pending_changes/gh-520-spaceship-nan-eval-order.txt new file mode 100644 index 0000000000..3781c49b40 --- /dev/null +++ b/src/pending_changes/gh-520-spaceship-nan-eval-order.txt @@ -0,0 +1,25 @@ +//allowed_words: NaN +[GH #520] +Lowering of <=> and the evaluation of its operands + +For floating-point operands, the lowering of <=> for the C-generating back +end tested the right operand for a NaN before evaluating the left operand, +using a short-circuit "||". When the right operand was a NaN, the left +operand was therefore not evaluated, and its side effects were lost. Also, +an operand without side effects could be read by the NaN test before the +other operand was evaluated, and by the comparisons after it. If the other +operand changed it, the result could then be "equivalent" for a NaN. For +example, with --c++20: + + #include + + float x(), y(); + auto f() { return x() <=> y(); } // Previously x() was not called when + // y() returned a NaN. + + float g, h(); // h() modifies g. + auto k() { return h() <=> g; } // Previously could be "equivalent" + // when h() stores a NaN into g. + +Each operand is now evaluated exactly once, and the NaN test and the +comparisons use the same operand values. diff --git a/tests/tests/regressions/.spaceship-nan-eval-order.rto/default.1.1.txt b/tests/tests/regressions/.spaceship-nan-eval-order.rto/default.1.1.txt new file mode 100644 index 0000000000..c7de245002 --- /dev/null +++ b/tests/tests/regressions/.spaceship-nan-eval-order.rto/default.1.1.txt @@ -0,0 +1 @@ +eccp -DTEST_NUMBER=1 --c++20 --g++ Test_name.c diff --git a/tests/tests/regressions/.spaceship-nan-eval-order.rto/default.1.2.txt b/tests/tests/regressions/.spaceship-nan-eval-order.rto/default.1.2.txt new file mode 100644 index 0000000000..ff2642c6d5 --- /dev/null +++ b/tests/tests/regressions/.spaceship-nan-eval-order.rto/default.1.2.txt @@ -0,0 +1 @@ +./a.out diff --git a/tests/tests/regressions/spaceship-nan-eval-order.sft.cpp b/tests/tests/regressions/spaceship-nan-eval-order.sft.cpp new file mode 100644 index 0000000000..f7d39656ed --- /dev/null +++ b/tests/tests/regressions/spaceship-nan-eval-order.sft.cpp @@ -0,0 +1,59 @@ +//type:rp +//options_all:--c++20 --g++ + +// Lowering of <=> must evaluate both operands exactly once, and must compare +// the same operand values in the NaN test and in the ordering tests. + +#include + +float nan_value() { return __builtin_nanf(""); } + +int calls; +float one() { ++calls; return 1.0f; } + +float g; +float set_g_to_nan() { g = nan_value(); return 0.0f; } + +int main() { + int failures = 0; + + // The left operand is evaluated when the right operand is a NaN. + calls = 0; + if ((one() <=> nan_value()) != std::partial_ordering::unordered) { + failures |= 1; + } // if + if (calls != 1) failures |= 2; + + // The right operand is evaluated when the left operand is a NaN. + calls = 0; + if ((nan_value() <=> one()) != std::partial_ordering::unordered) { + failures |= 4; + } // if + if (calls != 1) failures |= 8; + + // g is read either before set_g_to_nan() (less) or after it (unordered), + // never both. + g = 1.0f; + std::partial_ordering r = set_g_to_nan() <=> g; + if (r != std::partial_ordering::less && + r != std::partial_ordering::unordered) { + failures |= 16; + } // if + g = 1.0f; + r = g <=> set_g_to_nan(); + if (r != std::partial_ordering::greater && + r != std::partial_ordering::unordered) { + failures |= 32; + } // if + + // Finite operands give less, equivalent, and greater, both with operands + // that are copied to temporaries (one()) and with one that is not (g). + calls = 0; + g = 2.0f; + if ((one() <=> g) != std::partial_ordering::less) failures |= 64; + if ((one() <=> one()) != std::partial_ordering::equivalent) failures |= 64; + if ((g <=> one()) != std::partial_ordering::greater) failures |= 64; + if (calls != 4) failures |= 128; + + return failures; +}