First of all, it's great to support integer(16). But printing some numbers from range 2^63 to 2^64-1 with this snippet : program p print *, 9000000000000000000_16 print *, huge(1_8) print *, 2_16**63 print *, 10000000000000000000_16 print *, 18446744065119617024_16 print *, 2_16**64 - 1 print *, 2_16**64 print *, 20000000000000000000_16 end yields a wrong output for gfortran 5.1.1 (on SUSE Linux 13.2, 64 bit) 9000000000000000000 9223372036854775807 -9223372036854775808 -8446744073709551616 -8589934592 -1 18446744073709551616 20000000000000000000 Whereas with gfortran 4.9.0 and 4.8.3 the printed numbers are correct: 9000000000000000000 9223372036854775807 9223372036854775808 10000000000000000000 18446744065119617024 18446744073709551615 18446744073709551616 20000000000000000000 --- Same issue for the corresponding negative range : program p print *, -9000000000000000000_16 print *, -huge(1_8) print *, -(2_16**63) print *, -10000000000000000000_16 print *, -18446744065119617024_16 print *, -(2_16**64 - 1) print *, -(2_16**64) print *, -20000000000000000000_16 end Result with gfortran 5.1.1 : -9000000000000000000 -9223372036854775807 9223372036854775808 8446744073709551616 8589934592 1 -18446744073709551616 -20000000000000000000 Result with gfortran 4.9.0 and 4.8.3 : -9000000000000000000 -9223372036854775807 -9223372036854775808 -10000000000000000000 -18446744065119617024 -18446744073709551615 -18446744073709551616 -20000000000000000000
Of course, some computations are wrong, too. For example : program p integer(8), parameter :: z = huge(1_8) print *, 2_16 * z print *, 2 * int(z, 16) print *, 2_16 * int(z, 16) print *, int(2, 16) * int(z, 16) end Result with gfortran 5.1.1 : -2 -2 -2 -2 Result with gfortran 4.9.0 and 4.8.3 : 18446744073709551614 18446744073709551614 18446744073709551614 18446744073709551614 --- Or this variation : program p integer(16), parameter :: z = huge(1_8) print *, 2_16 * z print *, 2 * int(z, 16) print *, 2_16 * int(z, 16) print *, int(2, 16) * int(z, 16) end Result with gfortran 5.1.1 : -2 -2 -2 -2 Result with gfortran 4.9.0 and 4.8.3 : 18446744073709551614 18446744073709551614 18446744073709551614 18446744073709551614
Confirmed. Wow something broke here. I won't have tome until next week to start a regression hunt. Using -fdump-tree-original, one can see the different resulting constants, so it is something in the front-end.
The regression occurred between revision r210073 (2014-05-05, OK) and r210124 (2014-05-06, wrong output). I suspect r210113: Merge in wide-int.
GCC 5.2 is being released, adjusting target milestone to 5.3.
thanks > On Jul 16, 2015, at 5:12 AM, rguenth at gcc dot gnu.org <gcc-bugzilla@gcc.gnu.org> wrote: > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66311 > > Richard Biener <rguenth at gcc dot gnu.org> changed: > > What |Removed |Added > ---------------------------------------------------------------------------- > Target Milestone|5.2 |5.3 > > --- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> --- > GCC 5.2 is being released, adjusting target milestone to 5.3. > > -- > You are receiving this mail because: > You are on the CC list for the bug.
Definitely a middle-end issue. We get it right on the front-end, the variable initialization gets screwed when translated later. Reduced testcase: $ cat a.f90 program test integer(kind=16) :: z = 18446744073709551614_16 print *, z end $ ./usr/local/bin/gfortran -fdump-parse-tree -fdump-tree-original a.f90 && ./a.out -2 The "parse tree" clearly shows that z has value of 18446744073709551614_16. But in the a.f90.003t.original, we get the wrong initialization: static integer(kind=16) z = -2; I tracked the issue to the two calls to wi::from_mpz() and wide_int_to_tree() in gcc/fortran/trans-const.c:gfc_conv_mpz_to_tree(). There the mpz value (which is correct) in translated into: wide_int_storage = { val = ([0] = -2, [1] = 0, [2] = 56) len = 1 precision = 128 } which appears to be where the -2 comes from. Probably not seen outside Fortran because people don't use this conversion routine in C (because there is not way to input literal __int128 values in C). Can someone familiar with wide-int look into it?
(In reply to Francois-Xavier Coudert from comment #6) > I tracked the issue to the two calls to wi::from_mpz() and > wide_int_to_tree() in gcc/fortran/trans-const.c:gfc_conv_mpz_to_tree(). > There the mpz value (which is correct) in translated into: > > wide_int_storage = { > val = ([0] = -2, [1] = 0, [2] = 56) > len = 1 > precision = 128 > } > > which appears to be where the -2 comes from. And in wi::from_mpz it probably comes from the mpz_sizeinbytes and mpz_export functions not paying attention to the sign, as stated in the documentation: https://gmplib.org/manual/Integer-Import-and-Export.html > The sign of op is ignored, just the absolute value is exported. An application > can use mpz_sgn to get the sign and handle it as desired. (see Integer > Comparisons) https://gmplib.org/manual/Miscellaneous-Integer-Functions.html#Miscellaneous-Integer-Functions > The sign of op is ignored, just the absolute value is used.
Created attachment 36121 [details] Beginning of a patch The existing code mixes gmp allocation with wide_int allocation. With the patch, an extra sign bit needs to be written in the most significant word. I don't know how to avoid the memory management nightmare.
I've audited the patch for the memory management nightmares; we are safe with it.
I have audited the patch for the non memory management issues and it is approved. thanks for doing this. kenny On 08/04/2015 07:38 AM, mikael at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66311 > > --- Comment #8 from Mikael Morin <mikael at gcc dot gnu.org> --- > Created attachment 36121 [details] > --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=36121&action=edit > Beginning of a patch > > The existing code mixes gmp allocation with wide_int allocation. > With the patch, an extra sign bit needs to be written in the most significant > word. > I don't know how to avoid the memory management nightmare. >
(In reply to Kenneth Zadeck from comment #10) > I have audited the patch for the non memory management issues and it is > approved. Please don't apply just yet, I'd like to take a closer look. (Sorry for not doing it earlier.)
Created attachment 36128 [details] Alternative patch Here's an alternative patch. I haven't yet tested it beyond an expanded version of the testcase, but I think it's easier to follow if we separate the "val != valres" and "need an extra zero block" logic.
(In reply to rsandifo@gcc.gnu.org from comment #12) > Here's an alternative patch. I haven't yet tested it beyond > an expanded version of the testcase, but I think it's easier > to follow if we separate the "val != valres" and "need an extra > zero block" logic. I can confirm that the attached patch fixes all Fortran testcases reported in this thread.
(In reply to rsandifo@gcc.gnu.org from comment #12) > Created attachment 36128 [details] > Alternative patch > > Here's an alternative patch. I haven't yet tested it beyond > an expanded version of the testcase, but I think it's easier > to follow if we separate the "val != valres" and "need an extra > zero block" logic. Looks cleaner indeed. :-) I'm still worried by the case where x is negative and fits in wide_int, but its absolute value doesn't. It's somehow the same as this case, except that the boundary is at the size of wide_int (whatever it is) instead of 64 bits (the size of HOST_WIDE_INT) in this case. Not sure it matters.
Created attachment 36129 [details] Fortran testcase Attached is a Fortran testcase, ready for inclusion in gcc/testsuite/gfortran.dg Please commit it along with the patch, when the issue is fixed.
(In reply to Mikael Morin from comment #14) > (In reply to rsandifo@gcc.gnu.org from comment #12) > > Created attachment 36128 [details] > > Alternative patch > > > > Here's an alternative patch. I haven't yet tested it beyond > > an expanded version of the testcase, but I think it's easier > > to follow if we separate the "val != valres" and "need an extra > > zero block" logic. > > Looks cleaner indeed. :-) > I'm still worried by the case where x is negative and fits in wide_int, but > its absolute value doesn't. > It's somehow the same as this case, except that the boundary is at the size > of wide_int (whatever it is) instead of 64 bits (the size of HOST_WIDE_INT) > in this case. > Not sure it matters. Well, in a sense there's no such wide_int, since wide_ints don't have an inherent sign. If a wide_int has precision N, the absolute value of the minimum signed value is 1<<(N-1). We'd read that in the same way as we'd read an unsigned N-bit integer with value 1<<(N-1), so the value would fit at that stage. If the mpz is negative we then negate the wide_int, which for the minimum signed value is a no-op. We'd end up with an N-bit integer in which only bit N-1 is set. The problem in the testcase is more to do with the decision that, if an N-bit wide_int is represented with M<N bits, the other N-M bits are implicitly sign extensions of bit M-1. That's a pretty arbitrary choice. If we'd decided that the other bits were implicitly zero then no patch would be needed. (But of course we'd then have had to write the other wide_int code differently, and would have a less efficient representation of "negative" (or large positive) numbers.)
Author: rsandifo Date: Wed Aug 5 14:23:42 2015 New Revision: 226632 URL: https://gcc.gnu.org/viewcvs?rev=226632&root=gcc&view=rev Log: gcc/ PR middle-end/66311 * wide-int.cc (wi::from_mpz): Make sure that absolute mpz value is zero- rather than sign-extended. gcc/testsuite/ 2015-08-05 Francois-Xavier Coudert <fxcoudert@gcc.gnu.org> PR middle-end/66311 * gfortran.dg/pr66311.f90: New file. Added: trunk/gcc/testsuite/gfortran.dg/pr66311.f90 Modified: trunk/gcc/ChangeLog trunk/gcc/testsuite/ChangeLog trunk/gcc/wide-int.cc
Fixed on trunk. I'll wait a few days to see whether there's any unexpected fallout before backporting to gcc 5.
Still waiting for backport.
Author: mrs Date: Sat Oct 17 03:55:03 2015 New Revision: 228932 URL: https://gcc.gnu.org/viewcvs?rev=228932&root=gcc&view=rev Log: 2015-10-16 Richard Sandiford <richard.sandiford@arm.com> PR middle-end/66311 * wide-int.cc (wi::from_mpz): Make sure that absolute mpz value is zero- rather than sign-extended. testsuite: 2015-10-16 Francois-Xavier Coudert <fxcoudert@gcc.gnu.org> PR middle-end/66311 * gfortran.dg/pr66311.f90: New file. Added: branches/gcc-5-branch/gcc/testsuite/gfortran.dg/pr66311.f90 Modified: branches/gcc-5-branch/gcc/ChangeLog branches/gcc-5-branch/gcc/testsuite/ChangeLog branches/gcc-5-branch/gcc/wide-int.cc
Fixed.
I see the testcase fail at execution time for arm* targets, and it passes in aarch64* targets. I'm using qemu in both cases, if that matters.