[Bug fortran/96859] New: Wrong answer with intrinsic merge_bits

[Bug fortran/96859] New: Wrong answer with intrinsic merge_bits

[zhen.xu@compiler-dev.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96859

            Bug ID: 96859
           Summary: Wrong answer with intrinsic merge_bits
           Product: gcc
           Version: 10.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
          Assignee: unassigned at gcc dot gnu.org
          Reporter: zhen.xu@compiler-dev.com
  Target Milestone: ---

wrong result with compiled code such as:

merge_bits(32767_2, o'1234567', 32767_2)
merge_bits(o'1234567', 32767_2, o'1234567')
merge_bits(32767_2, o'1234567', b'010101')
merge_bits(32767_2, o'1234567', z'12345678')

[Bug fortran/96859] Wrong answer with intrinsic merge_bits

[dominiq at lps dot ens.fr]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96859

Dominique d'Humieres <dominiq at lps dot ens.fr> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2020-08-31
             Status|UNCONFIRMED                 |WAITING
     Ever confirmed|0                           |1

--- Comment #1 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
What results do you expect?

[Bug fortran/96859] Wrong answer with intrinsic merge_bits

[kargl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96859

kargl at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kargl at gcc dot gnu.org

--- Comment #2 from kargl at gcc dot gnu.org ---
gfortran appears to be correct.  Writing an actual program with your
examples, I have

program foo
   print '(I0,1X,I0)', &
   &  merge_bits(32767_2,o'1234567',32767_2), &
   &  ior(iand(32767_2,32767_2),iand(o'1234567',not(32767_2)))

   print '(I0,1X,I0)', &
   &  merge_bits(o'1234567',32767_2,o'1234567'), &
   &
ior(iand(o'1234567',int(o'1234567',2)),iand(32767_2,not(int(o'1234567',2))))

   print '(I0,1X,I0)', &
   &  merge_bits(32767_2,o'1234567',b'010101'), &
   &  ior(iand(32767_2,b'010101'),iand(o'1234567', not(int(b'010101',2))))

   print '(I0,1X,I0)', &
   &  merge_bits(32767_2,o'1234567',z'12345678'), &
   &  ior(iand(32767_2,z'12345678'), iand(o'1234567',not(int( z'12345678',2))))
end program foo

[Bug fortran/96859] Wrong answer with intrinsic merge_bits

[zhen.xu@compiler-dev.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96859

--- Comment #3 from zhen.xu@compiler-dev.com ---
(In reply to Dominique d'Humieres from comment #1)
> What results do you expect?

merge_bits(32767_2, o'1234567', 32767_2)
merge_bits(o'1234567', 32767_2, o'1234567')
merge_bits(32767_2, o'1234567', b'010101')
merge_bits(32767_2, o'1234567', z'12345678')


The four corresponding results should be:
32767
32767
14711
32639

I checked the Fortran 2008 standard and confirmed the results with hand
calculation.

[Bug fortran/96859] Wrong answer with intrinsic merge_bits

[zhen.xu@compiler-dev.com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96859

--- Comment #4 from zhen.xu@compiler-dev.com ---
(In reply to kargl from comment #2)
> gfortran appears to be correct.  Writing an actual program with your
> examples, I have
> 
> program foo
>    print '(I0,1X,I0)', &
>    &  merge_bits(32767_2,o'1234567',32767_2), &
>    &  ior(iand(32767_2,32767_2),iand(o'1234567',not(32767_2)))
> 
>    print '(I0,1X,I0)', &
>    &  merge_bits(o'1234567',32767_2,o'1234567'), &
>    &
> ior(iand(o'1234567',int(o'1234567',2)),iand(32767_2,not(int(o'1234567',2))))
> 
>    print '(I0,1X,I0)', &
>    &  merge_bits(32767_2,o'1234567',b'010101'), &
>    &  ior(iand(32767_2,b'010101'),iand(o'1234567', not(int(b'010101',2))))
> 
>    print '(I0,1X,I0)', &
>    &  merge_bits(32767_2,o'1234567',z'12345678'), &
>    &  ior(iand(32767_2,z'12345678'), iand(o'1234567',not(int(
> z'12345678',2))))
> end program foo

I compiled your code with gfortran-10, getting result:
-1 -1
-1 -1
-18057 -18057
-129 -129


The right one should be:
32767 32767
32767 32767
14711 14711
32639 32639

[Bug fortran/96859] Wrong answer with intrinsic merge_bits

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96859

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org
             Status|WAITING                     |NEW

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I think this boils down to:
program foo
  print *, int(o'1234567',2), int(o'1234567',4)
end program foo
I believe ifort prints
14711 342391
while gfortran prints
-18057 342391
Since 01234567 in C is 0x53977, after that is cast to 16-bit integer that
should be 0x3977 and thus 14711.

[Bug fortran/96859] Wrong answer with intrinsic merge_bits

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96859

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Untested fix:
--- gcc/fortran/check.c.jj      2020-08-24 10:00:01.424256990 +0200
+++ gcc/fortran/check.c 2020-09-01 15:06:45.428938642 +0200
@@ -429,9 +429,9 @@ gfc_boz2int (gfc_expr *x, int kind)
       /* Clear first two bits.  */
       else
        {
-         if (buf[0] == '4' || buf[0] == '6')
+         if (buf[0] == '2' || buf[0] == '4' || buf[0] == '6')
            buf[0] = '0';
-         else if (buf[0] == '5' || buf[0] == '7')
+         else if (buf[0] == '3' || buf[0] == '5' || buf[0] == '7')
            buf[0] = '1';
        }
     }

[Bug fortran/96859] Wrong answer with intrinsic merge_bits

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96859

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 49164
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=49164&action=edit
gcc11-pr96859.patch

Full untested patch.

[Bug fortran/96859] Wrong answer with intrinsic merge_bits

[sgk at troutmask dot apl.washington.edu]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96859

--- Comment #8 from Steve Kargl <sgk at troutmask dot apl.washington.edu> ---
On Tue, Sep 01, 2020 at 12:52:49PM +0000, jakub at gcc dot gnu.org wrote:
> 
> --- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> I think this boils down to:
> program foo
>   print *, int(o'1234567',2), int(o'1234567',4)
> end program foo
> I believe ifort prints
> 14711 342391
> while gfortran prints
> -18057 342391
> Since 01234567 in C is 0x53977, after that is cast to 16-bit integer that
> should be 0x3977 and thus 14711.
> 

The Fortran standard states how the BOZ string of bits
is padded or truncated if it contains too few or too
many bits.  See F2018, 16.3.3.  Also no that the 
handling of the sign bit is processor dependent.

--
Steve

[Bug fortran/96859] Wrong answer with intrinsic merge_bits

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96859

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I've read that.  But I think in this case it is an obvious bug (just, what I've
missed in the patch, there is another copy of the same bug in another routine).
The
      /* Clear first bit.  */
      if (kind == 1 || kind == 4 || kind == 16)
        {
          if (buf[0] == '4')
            buf[0] = '0';
          else if (buf[0] == '5')
            buf[0] = '1';
          else if (buf[0] == '6')
            buf[0] = '2';
          else if (buf[0] == '7')
            buf[0] = '3';
        }
part looks correct, for kind 1, 4 and 16 the calculated len times 3 is 1 larger
than the number of bits it needs, so the above ensures that the first digit is
0-3 even if it is 4-7 by subtracting 4.
But the:
      /* Clear first two bits.  */
      else
        {
          if (buf[0] == '4' || buf[0] == '6')
            buf[0] = '0';
          else if (buf[0] == '5' || buf[0] == '7')
            buf[0] = '1';
        }
which is needed for kind 2 and 8, when he calculated len times 3 is 2 larger
than the number of bits it needs, is only correct for digits 0-1 an 4-7, for
which it ensures the digit is 0 if it is even and 1 if it is odd.  But 2 and 3
are kept as is, while they don't fit into 1 bit.

[Bug fortran/96859] Wrong answer with intrinsic merge_bits

[sgk at troutmask dot apl.washington.edu]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96859

--- Comment #10 from Steve Kargl <sgk at troutmask dot apl.washington.edu> ---
On Tue, Sep 01, 2020 at 03:20:20PM +0000, jakub at gcc dot gnu.org wrote:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96859
> 
> --- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> I've read that.  But I think in this case it is an obvious bug (just, what I've
> missed in the patch, there is another copy of the same bug in another routine).
> The
>       /* Clear first bit.  */
>       if (kind == 1 || kind == 4 || kind == 16)
>         {
>           if (buf[0] == '4')
>             buf[0] = '0';
>           else if (buf[0] == '5')
>             buf[0] = '1';
>           else if (buf[0] == '6')
>             buf[0] = '2';
>           else if (buf[0] == '7')
>             buf[0] = '3';
>         }
> part looks correct, for kind 1, 4 and 16 the calculated len times 3 is 1 larger
> than the number of bits it needs, so the above ensures that the first digit is
> 0-3 even if it is 4-7 by subtracting 4.
> But the:
>       /* Clear first two bits.  */
>       else
>         {
>           if (buf[0] == '4' || buf[0] == '6')
>             buf[0] = '0';
>           else if (buf[0] == '5' || buf[0] == '7')
>             buf[0] = '1';
>         }
> which is needed for kind 2 and 8, when he calculated len times 3 is 2 larger
> than the number of bits it needs, is only correct for digits 0-1 an 4-7, for
> which it ensures the digit is 0 if it is even and 1 if it is odd.  But 2 and 3
> are kept as is, while they don't fit into 1 bit.
> 

It is certainly possible I have/had an off-by-one in handwritten
conversions I was doing; in particular, I don't often work with
octal numbers.

[Bug fortran/96859] Wrong answer with intrinsic merge_bits

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96859

--- Comment #11 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:b567d3bd302933adb253aba9069fd8120c485441

commit r11-2978-gb567d3bd302933adb253aba9069fd8120c485441
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Sep 2 12:18:46 2020 +0200

    fortran: Fix o'...' boz to integer/real conversions [PR96859]

    The standard says that excess digits from boz are truncated.
    For hexadecimal or binary, the routines copy just the number of digits
    that will be needed, but for octal we copy number of digits that
    contain one extra bit (for 8-bit, 32-bit or 128-bit, i.e. kind 1, 4 and 16)
    or two extra bits (for 16-bit or 64-bit, i.e. kind 2 and 8).
    The clearing of the first bit is done correctly by changing the first digit
    if it is 4-7 to one smaller by 4 (i.e. modulo 4).
    The clearing of the first two bits is done by changing 4 or 6 to 0
    and 5 or 7 to 1, which is incorrect, because we really want to change the
    first digit to 0 if it was even, or to 1 if it was odd, so digits
    2 and 3 are mishandled by keeping them as is, rather than changing 2 to 0
    and 3 to 1.

    2020-09-02  Jakub Jelinek  <jakub@redhat.com>

            PR fortran/96859
            * check.c (gfc_boz2real, gfc_boz2int): When clearing first two
bits,
            change also '2' to '0' and '3' to '1' rather than just handling '4'
            through '7'.

            * gfortran.dg/pr96859.f90: New test.

[Bug fortran/96859] Wrong answer with intrinsic merge_bits

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96859

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:e24dbeeb7864dc4c673f57383c3fa329d1d16e71

commit r10-8729-ge24dbeeb7864dc4c673f57383c3fa329d1d16e71
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Sep 2 12:18:46 2020 +0200

    fortran: Fix o'...' boz to integer/real conversions [PR96859]

    The standard says that excess digits from boz are truncated.
    For hexadecimal or binary, the routines copy just the number of digits
    that will be needed, but for octal we copy number of digits that
    contain one extra bit (for 8-bit, 32-bit or 128-bit, i.e. kind 1, 4 and 16)
    or two extra bits (for 16-bit or 64-bit, i.e. kind 2 and 8).
    The clearing of the first bit is done correctly by changing the first digit
    if it is 4-7 to one smaller by 4 (i.e. modulo 4).
    The clearing of the first two bits is done by changing 4 or 6 to 0
    and 5 or 7 to 1, which is incorrect, because we really want to change the
    first digit to 0 if it was even, or to 1 if it was odd, so digits
    2 and 3 are mishandled by keeping them as is, rather than changing 2 to 0
    and 3 to 1.

    2020-09-02  Jakub Jelinek  <jakub@redhat.com>

            PR fortran/96859
            * check.c (gfc_boz2real, gfc_boz2int): When clearing first two
bits,
            change also '2' to '0' and '3' to '1' rather than just handling '4'
            through '7'.

            * gfortran.dg/pr96859.f90: New test.

    (cherry picked from commit b567d3bd302933adb253aba9069fd8120c485441)

[Bug fortran/96859] Wrong answer with intrinsic merge_bits

[anlauf at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96859

anlauf at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |WAITING

--- Comment #13 from anlauf at gcc dot gnu.org ---
Anything left to do or can the PR be closed?

[Bug fortran/96859] Wrong answer with intrinsic merge_bits

[dominiq at lps dot ens.fr]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96859

Dominique d'Humieres <dominiq at lps dot ens.fr> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |RESOLVED
         Resolution|---                         |FIXED

--- Comment #14 from Dominique d'Humieres <dominiq at lps dot ens.fr> ---
> Anything left to do or can the PR be closed?

I think so. Please reopen if I missed something.

[Bug fortran/96859] Wrong answer with intrinsic merge_bits

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96859

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |11.3