The following program should print: 0.001914 0.630538 With "g++ -frounding-math", it prints instead: -8023756970655744.000000 0.872496 This bug is present in trunk, and since gcc 12.1, and does not appear to be platform specific. Compiler explorer link: https://godbolt.org/z/aMhcYcY66 #include <cstdio> float xs[] = {0.001914, 0.630539}; int main() { std::size_t size = sizeof xs / sizeof xs[0]; for (std::size_t i = 0; i != size; ++i) { std::printf("%f\n", xs[i]); } }
With -frounding-math: xs: .long -639366012 .long 1063214053 .long 536561674 .long 1071918432 without: xs: .long 989519663 .long 1059154689 it looks like we fail to convert the double constant to single precision and then end up outputting the double precision constants ... The C frontend works.
Started with r12-4764-ga84b9d5373c7e67fd0ab2a
I think the fold-const.cc change is right though. I wonder if for constant evaluation (constexpr, constinit) we shouldn't arrange for those to be evaluated with temporarily -fno-rounding-math, I think C uses fold_init and its START_FOLD_INIT ... END_FOLD_INIT for this purpose.. And otherwise perhaps we want dynamic initialization and do the conversion at runtime? Or disable the -frounding-math for all initializer folding? What we emit is definitely wrong, Variable which claims to have 8 bytes in size but actually has 16 under the hood, with constants in different mode.
(In reply to Jakub Jelinek from comment #3) > I think the fold-const.cc change is right though. > I wonder if for constant evaluation (constexpr, constinit) we shouldn't > arrange for those to be evaluated with temporarily -fno-rounding-math, I > think C uses > fold_init and its START_FOLD_INIT ... END_FOLD_INIT for this purpose.. > And otherwise perhaps we want dynamic initialization and do the conversion > at runtime? > Or disable the -frounding-math for all initializer folding? > What we emit is definitely wrong, > Variable which claims to have 8 bytes in size but actually has 16 under the > hood, with constants in different mode. We should have ICEd emitting the constant. And yes, I think -frounding-math should be disabled for constinit initializer folding (and possibly whether it is const or not should not depend on -frounding-math).
Ah, cp/constexpr.cc already uses fold_binary_initializer_loc if -fconstexpr-fp-except. That will turn the -frounding-math temporarily off for binary operations. For this PR guess we need to use fold_init or fold_build1_initializer_loc instead of fold or fold_build1 in the NOP_EXPR handling under the same conditions. Plus of course we need to figure out how to fix the issue if we didn't turn those off (we shouldn't consider the initializer constant then but evaluate dynamically).
output_constant gets called with {(float) 1.91399999999999990279997419406754488591104745864868164062e-3, (float) 6.305389999999999606217215841752476990222930908203125e-1} it then eventually does /* Eliminate any conversions since we'll be outputting the underlying constant. */ while (CONVERT_EXPR_P (exp) || TREE_CODE (exp) == NON_LVALUE_EXPR || TREE_CODE (exp) == VIEW_CONVERT_EXPR) { HOST_WIDE_INT type_size = int_size_in_bytes (TREE_TYPE (exp)); HOST_WIDE_INT op_size = int_size_in_bytes (TREE_TYPE (TREE_OPERAND (exp, 0))); /* Make sure eliminating the conversion is really a no-op, except with VIEW_CONVERT_EXPRs to allow for wild Ada unchecked conversions and union types to allow for Ada unchecked unions. */ if (type_size > op_size && TREE_CODE (exp) != VIEW_CONVERT_EXPR && TREE_CODE (TREE_TYPE (exp)) != UNION_TYPE) /* Keep the conversion. */ break; else exp = TREE_OPERAND (exp, 0); } where we strip conversions with type_size < op_size (aka float from double). For float conversions not sure if just keying on type size is good enough though (ibm double double vs long double 128 for example). Fixing that "improves" the behavior to t.ii:1:34: error: initializer for floating value is not a floating constant 1 | float xs[] = {0.001914, 0.630539}; | ^ t.ii:1:34: error: initializer for floating value is not a floating constant aka from wrong-code to rejects-valid. diff --git a/gcc/varasm.cc b/gcc/varasm.cc index cd0cd88321c..e6ab581dc5f 100644 --- a/gcc/varasm.cc +++ b/gcc/varasm.cc @@ -5202,7 +5202,7 @@ output_constant (tree exp, unsigned HOST_WIDE_INT size, unsigned int align, /* Make sure eliminating the conversion is really a no-op, except with VIEW_CONVERT_EXPRs to allow for wild Ada unchecked conversions and union types to allow for Ada unchecked unions. */ - if (type_size > op_size + if (type_size != op_size && TREE_CODE (exp) != VIEW_CONVERT_EXPR && TREE_CODE (TREE_TYPE (exp)) != UNION_TYPE) /* Keep the conversion. */ note that for integral and pointer types we do cst = expand_expr (exp, NULL_RTX, VOIDmode, EXPAND_INITIALIZER); if (reverse) cst = flip_storage_order (TYPE_MODE (TREE_TYPE (exp)), cst); if (!assemble_integer (cst, MIN (size, thissize), align, 0)) error ("initializer for integer/fixed-point value is too complicated"); so we handle "narrowing" in a weird way. So in case FEs leave around nop-casts the following should be safer diff --git a/gcc/varasm.cc b/gcc/varasm.cc index cd0cd88321c..81f7288449c 100644 --- a/gcc/varasm.cc +++ b/gcc/varasm.cc @@ -5196,13 +5196,17 @@ output_constant (tree exp, unsigned HOST_WIDE_INT size, unsigned int align, || TREE_CODE (exp) == NON_LVALUE_EXPR || TREE_CODE (exp) == VIEW_CONVERT_EXPR) { - HOST_WIDE_INT type_size = int_size_in_bytes (TREE_TYPE (exp)); - HOST_WIDE_INT op_size = int_size_in_bytes (TREE_TYPE (TREE_OPERAND (exp, 0))); + tree type = TREE_TYPE (exp); + tree op_type = TREE_TYPE (TREE_OPERAND (exp, 0)); + HOST_WIDE_INT type_size = int_size_in_bytes (type); + HOST_WIDE_INT op_size = int_size_in_bytes (op_type); /* Make sure eliminating the conversion is really a no-op, except with VIEW_CONVERT_EXPRs to allow for wild Ada unchecked conversions and union types to allow for Ada unchecked unions. */ - if (type_size > op_size + if ((type_size > op_size + || (TYPE_MAIN_VARIANT (type) != TYPE_MAIN_VARIANT (op_type) + && FLOAT_TYPE_P (type))) && TREE_CODE (exp) != VIEW_CONVERT_EXPR && TREE_CODE (TREE_TYPE (exp)) != UNION_TYPE) /* Keep the conversion. */ The real fix is of course in the frontend, the above is just a safety net.
For the safety net I'd compare TYPE_MODE of the SCALAR_FLOAT_TYPE_Ps, that is what matters for those whether it is a noop conversion or needs actually some runtime adjustment.
GCC 12.3 is being released, retargeting bugs to GCC 12.4.
This seems to be fixed on trunk. Jakub, can you bisect what fixed it?
r14-1498-ge7cc4d703bceb9095316c106eba0d1939c6c8044
The trunk branch has been updated by Jason Merrill <jason@gcc.gnu.org>: https://gcc.gnu.org/g:d49780c08aade447953bfe4e877d386f5757f165 commit r14-8813-gd49780c08aade447953bfe4e877d386f5757f165 Author: Jason Merrill <jason@redhat.com> Date: Mon Feb 5 15:59:45 2024 -0500 c++: -frounding-math test [PR109359] This test was fixed by the patch for PR95226, but that patch had no testcase so let's add this one. PR c++/109359 gcc/testsuite/ChangeLog: * g++.dg/ext/frounding-math1.C: New test.
The releases/gcc-12 branch has been updated by Jason Merrill <jason@gcc.gnu.org>: https://gcc.gnu.org/g:470f501f31a4bdb9fa04c691ca7db2915ac3ae5b commit r12-10136-g470f501f31a4bdb9fa04c691ca7db2915ac3ae5b Author: Jason Merrill <jason@redhat.com> Date: Thu Jun 1 14:41:07 2023 -0400 varasm: check float size [PR109359] In PR95226, the testcase was failing because we tried to output_constant a NOP_EXPR to float from a double REAL_CST, and so we output a double where the caller wanted a float. That doesn't happen anymore, but with the output_constant hunk we will ICE in that situation rather than emit the wrong number of bytes. Part of the problem was that initializer_constant_valid_p_1 returned true for that NOP_EXPR, because it compared the sizes of integer types but not floating-point types. So the C++ front end assumed it didn't need to fold the initializer. This also fixed the test for PR109359. PR c++/95226 PR c++/109359 gcc/ChangeLog: * varasm.cc (output_constant) [REAL_TYPE]: Check that sizes match. (initializer_constant_valid_p_1): Compare float precision. gcc/testsuite/ChangeLog: * g++.dg/ext/frounding-math1.C: New test. (cherry picked from commit e7cc4d703bceb9095316c106eba0d1939c6c8044)
The releases/gcc-13 branch has been updated by Jason Merrill <jason@gcc.gnu.org>: https://gcc.gnu.org/g:9b8e82ab45d1ad976a824cfd7c9bd2640c8bc8e3 commit r13-8282-g9b8e82ab45d1ad976a824cfd7c9bd2640c8bc8e3 Author: Jason Merrill <jason@redhat.com> Date: Thu Jun 1 14:41:07 2023 -0400 varasm: check float size [PR109359] In PR95226, the testcase was failing because we tried to output_constant a NOP_EXPR to float from a double REAL_CST, and so we output a double where the caller wanted a float. That doesn't happen anymore, but with the output_constant hunk we will ICE in that situation rather than emit the wrong number of bytes. Part of the problem was that initializer_constant_valid_p_1 returned true for that NOP_EXPR, because it compared the sizes of integer types but not floating-point types. So the C++ front end assumed it didn't need to fold the initializer. This also fixed the test for PR109359. PR c++/95226 PR c++/109359 gcc/ChangeLog: * varasm.cc (output_constant) [REAL_TYPE]: Check that sizes match. (initializer_constant_valid_p_1): Compare float precision. gcc/testsuite/ChangeLog: * g++.dg/ext/frounding-math1.C: New test. (cherry picked from commit e7cc4d703bceb9095316c106eba0d1939c6c8044)
Fixed for 12.4/13.3/14.0.