diff mbox series

[fortran] PR90577, PR90578 - FAIL: gfortran.dg/lrshift_1.f90 & Wrong code with LSHIFT and optimization

Message ID 5D02C51F.8030503@gmx.de
State New
Headers show
Series [fortran] PR90577, PR90578 - FAIL: gfortran.dg/lrshift_1.f90 & Wrong code with LSHIFT and optimization | expand

Commit Message

Harald Anlauf June 13, 2019, 9:50 p.m. UTC 
Dear all,

it was already discussed in the above PRs that the testcase
lrshift_1.f90 needs to be corrected because it invokes negative
values of the SHIFT argument.  This is fixed, as well as the
related documentation, which did not match e.g. the Fortran standard.

Looking closer at the actual implementation, it also turned out that the
runtime implementation of SHIFTA/RSHIFT gave different (=wrong) results
when SHIFT==BIT_SIZE(arg1).  This is fixed and tested by the attached
patches.

I did not implement any run-time checks for the SHIFT argument.
All previous undefined behavior stays the same.

OK for trunk?

Thanks,
Harald

2019-06-13  Harald Anlauf  <anlauf@gmx.de>

	PR fortran/90577
	PR fortran/90578
	* trans-intrinsic.c (gfc_conv_intrinsic_shift): Properly
	distinguish logical/arithmetic shifts.
	* intrinsic.texi: Update documentation for SHIFTR/SHIFTL/SHIFTA
	(Fortran 2008) and LSHIFT/RSHIFT (GNU extensions).

2019-06-13  Harald Anlauf  <anlauf@gmx.de>

	PR fortran/90577
	PR fortran/90578
	* gfortran.dg/lrshift_1.f90: Adjust testcase.
	* gfortran.dg/shiftalr_3.f90: New testcase.

Index: gcc/testsuite/gfortran.dg/lrshift_1.f90
===================================================================
--- gcc/testsuite/gfortran.dg/lrshift_1.f90	(revision 272261)
+++ gcc/testsuite/gfortran.dg/lrshift_1.f90	(working copy)
@@ -10,7 +10,7 @@
          1, 2, 127, 128, 129, huge(i)/2, huge(i) /)

   do n = 1, size(i)
-    do j = -30, 30
+    do j = 0, 31
       if (lshift(i(n),j) /= c_lshift(i(n),j)) STOP 1
       if (rshift(i(n),j) /= c_rshift(i(n),j)) STOP 2
     end do
Index: gcc/testsuite/gfortran.dg/shiftalr_3.f90
===================================================================
--- gcc/testsuite/gfortran.dg/shiftalr_3.f90	(nonexistent)
+++ gcc/testsuite/gfortran.dg/shiftalr_3.f90	(working copy)
@@ -0,0 +1,42 @@
+! { dg-do run }
+!
+! Test shift intrinsics when the SHIFT argument equals BIT_SIZE(arg1).
+
+program test
+  implicit none
+  ! Test compile-time simplifications
+  if (ishft  (-1, 32) /=  0) stop 1 !  0 -> simplify_shift OK
+  if (ishft  (-1,-32) /=  0) stop 2 !  0 -> simplify_shift OK
+  if (shiftl (-1, 32) /=  0) stop 3 !  0 -> simplify_shift OK
+  if (shiftr (-1, 32) /=  0) stop 4 !  0 -> simplify_shift OK
+  if (shifta (-1, 32) /= -1) stop 5 ! -1 -> simplify_shift OK
+  if (rshift (-1, 32) /= -1) stop 6 ! -1 -> simplify_shift OK
+  if (lshift (-1, 32) /=  0) stop 7 !  0 -> simplify_shift OK
+  ! Test run-time
+  call foo (-1)
+contains
+  subroutine foo (n)
+    integer(4) :: i, j, k, n
+    integer, parameter :: bb = bit_size (n)
+    ! Test code generated by gfc_conv_intrinsic_ishft
+    i = ishft  (n, bb) ! Logical (left)  shift (Fortran 2008)
+    j = ishft  (n,-bb) ! Logical (right) shift (Fortran 2008)
+    if (i /= 0) stop 11
+    if (j /= 0) stop 12
+    ! Test code generated by gfc_conv_intrinsic_shift:
+    i = shiftl (n, bb) ! Logical    left  shift (Fortran 2008)
+    j = shiftr (n, bb) ! Logical    right shift (Fortran 2008)
+    k = shifta (n, bb) ! Arithmetic right shift (Fortran 2008)
+    if (i /=  0) stop 13
+    if (j /=  0) stop 14
+    if (k /= -1) stop 15
+    i = lshift (n, bb) ! Logical    left  shift (GNU extension)
+    j = rshift (n, bb) ! Arithmetic right shift (GNU extension)
+    if (i /=  0) stop 16
+    if (j /= -1) stop 17
+    do i = bb-1,bb
+       if (shifta (n, i) /= -1) stop 18
+       if (rshift (n, i) /= -1) stop 19
+    end do
+  end subroutine foo
+end program test

Comments

Steve Kargl June 13, 2019, 10:18 p.m. UTC | #1 
On Thu, Jun 13, 2019 at 11:50:23PM +0200, Harald Anlauf wrote:
> 
> it was already discussed in the above PRs that the testcase
> lrshift_1.f90 needs to be corrected because it invokes negative
> values of the SHIFT argument.  This is fixed, as well as the
> related documentation, which did not match e.g. the Fortran standard.
> 
> Looking closer at the actual implementation, it also turned out that the
> runtime implementation of SHIFTA/RSHIFT gave different (=wrong) results
> when SHIFT==BIT_SIZE(arg1).  This is fixed and tested by the attached
> patches.
> 
> I did not implement any run-time checks for the SHIFT argument.
> All previous undefined behavior stays the same.
> 
> OK for trunk?
> 

Looks good to me.
Harald Anlauf June 14, 2019, 6:44 p.m. UTC | #2 
Committed as svn rev. 272309.

Thanks for the quick review!

Harald

On 06/14/19 00:18, Steve Kargl wrote:
> On Thu, Jun 13, 2019 at 11:50:23PM +0200, Harald Anlauf wrote:
>>
>> it was already discussed in the above PRs that the testcase
>> lrshift_1.f90 needs to be corrected because it invokes negative
>> values of the SHIFT argument.  This is fixed, as well as the
>> related documentation, which did not match e.g. the Fortran standard.
>>
>> Looking closer at the actual implementation, it also turned out that the
>> runtime implementation of SHIFTA/RSHIFT gave different (=wrong) results
>> when SHIFT==BIT_SIZE(arg1).  This is fixed and tested by the attached
>> patches.
>>
>> I did not implement any run-time checks for the SHIFT argument.
>> All previous undefined behavior stays the same.
>>
>> OK for trunk?
>>
>
> Looks good to me.
>
diff mbox series

Patch

Index: gcc/fortran/intrinsic.texi
===================================================================
--- gcc/fortran/intrinsic.texi	(revision 272261)
+++ gcc/fortran/intrinsic.texi	(working copy)
@@ -9689,10 +9689,10 @@ 
 @table @asis
 @item @emph{Description}:
 @code{LSHIFT} returns a value corresponding to @var{I} with all of the
-bits shifted left by @var{SHIFT} places.  If the absolute value of
-@var{SHIFT} is greater than @code{BIT_SIZE(I)}, the value is undefined.
-Bits shifted out from the left end are lost; zeros are shifted in from
-the opposite end.
+bits shifted left by @var{SHIFT} places.  @var{SHIFT} shall be
+nonnegative and less than or equal to @code{BIT_SIZE(I)}, otherwise
+the result value is undefined.  Bits shifted out from the left end are
+lost; zeros are shifted in from the opposite end.

 This function has been superseded by the @code{ISHFT} intrinsic, which
 is standard in Fortran 95 and later, and the @code{SHIFTL} intrinsic,
@@ -12244,11 +12244,12 @@ 
 @table @asis
 @item @emph{Description}:
 @code{RSHIFT} returns a value corresponding to @var{I} with all of the
-bits shifted right by @var{SHIFT} places.  If the absolute value of
-@var{SHIFT} is greater than @code{BIT_SIZE(I)}, the value is undefined.
-Bits shifted out from the right end are lost. The fill is arithmetic: the
-bits shifted in from the left end are equal to the leftmost bit, which in
-two's complement representation is the sign bit.
+bits shifted right by @var{SHIFT} places.  @var{SHIFT} shall be
+nonnegative and less than or equal to @code{BIT_SIZE(I)}, otherwise
+the result value is undefined.  Bits shifted out from the right end
+are lost. The fill is arithmetic: the bits shifted in from the left
+end are equal to the leftmost bit, which in two's complement
+representation is the sign bit.

 This function has been superseded by the @code{SHIFTA} intrinsic, which
 is standard in Fortran 2008 and later.
@@ -12783,11 +12784,12 @@ 
 @table @asis
 @item @emph{Description}:
 @code{SHIFTA} returns a value corresponding to @var{I} with all of the
-bits shifted right by @var{SHIFT} places.  If the absolute value of
-@var{SHIFT} is greater than @code{BIT_SIZE(I)}, the value is undefined.
-Bits shifted out from the right end are lost. The fill is arithmetic: the
-bits shifted in from the left end are equal to the leftmost bit, which in
-two's complement representation is the sign bit.
+bits shifted right by @var{SHIFT} places.  @var{SHIFT} that be
+nonnegative and less than or equal to @code{BIT_SIZE(I)}, otherwise
+the result value is undefined.  Bits shifted out from the right end
+are lost. The fill is arithmetic: the bits shifted in from the left
+end are equal to the leftmost bit, which in two's complement
+representation is the sign bit.

 @item @emph{Standard}:
 Fortran 2008 and later
@@ -12823,10 +12825,10 @@ 
 @table @asis
 @item @emph{Description}:
 @code{SHIFTL} returns a value corresponding to @var{I} with all of the
-bits shifted left by @var{SHIFT} places.  If the absolute value of
-@var{SHIFT} is greater than @code{BIT_SIZE(I)}, the value is undefined.
-Bits shifted out from the left end are lost, and bits shifted in from
-the right end are set to 0.
+bits shifted left by @var{SHIFT} places.  @var{SHIFT} shall be
+nonnegative and less than or equal to @code{BIT_SIZE(I)}, otherwise
+the result value is undefined.  Bits shifted out from the left end are
+lost, and bits shifted in from the right end are set to 0.

 @item @emph{Standard}:
 Fortran 2008 and later
@@ -12862,10 +12864,10 @@ 
 @table @asis
 @item @emph{Description}:
 @code{SHIFTR} returns a value corresponding to @var{I} with all of the
-bits shifted right by @var{SHIFT} places.  If the absolute value of
-@var{SHIFT} is greater than @code{BIT_SIZE(I)}, the value is undefined.
-Bits shifted out from the right end are lost, and bits shifted in from
-the left end are set to 0.
+bits shifted right by @var{SHIFT} places.  @var{SHIFT} shall be
+nonnegative and less than or equal to @code{BIT_SIZE(I)}, otherwise
+the result value is undefined.  Bits shifted out from the right end
+are lost, and bits shifted in from the left end are set to 0.

 @item @emph{Standard}:
 Fortran 2008 and later
Index: gcc/fortran/trans-intrinsic.c
===================================================================
--- gcc/fortran/trans-intrinsic.c	(revision 272261)
+++ gcc/fortran/trans-intrinsic.c	(working copy)
@@ -6346,6 +6346,7 @@ 
 			  bool arithmetic)
 {
   tree args[2], type, num_bits, cond;
+  tree bigshift;

   gfc_conv_intrinsic_function_args (se, expr, args, 2);

@@ -6365,6 +6366,18 @@ 
   if (!arithmetic)
     se->expr = fold_convert (type, se->expr);

+  if (!arithmetic)
+    bigshift = build_int_cst (type, 0);
+  else
+    {
+      tree nonneg = fold_build2_loc (input_location, GE_EXPR,
+				     logical_type_node, args[0],
+				     build_int_cst (TREE_TYPE (args[0]), 0));
+      bigshift = fold_build3_loc (input_location, COND_EXPR, type, nonneg,
+				  build_int_cst (type, 0),
+				  build_int_cst (type, -1));
+    }
+
   /* The Fortran standard allows shift widths <= BIT_SIZE(I), whereas
      gcc requires a shift width < BIT_SIZE(I), so we have to catch this
      special case.  */
@@ -6373,7 +6386,7 @@ 
 			  args[1], num_bits);

   se->expr = fold_build3_loc (input_location, COND_EXPR, type, cond,
-			      build_int_cst (type, 0), se->expr);
+			      bigshift, se->expr);
 }

 /* ISHFT (I, SHIFT) = (abs (shift) >= BIT_SIZE (i))