[Patch, Fortran] PR fortran/46794: Fix ICE with powers of integers

[Patch, Fortran] PR fortran/46794: Fix ICE with powers of integers

[Daniel Kraft]

[-- Attachment #1: Type: text/plain, Size: 797 bytes --]

Hi all,

I'm (a little) back to gfortran ;)  The attached patch fixes the problem
of PR 46794 -- namely that when we call _gfortran_pow_i4_i4 even for
integer operands with kinds 1 / 2, the result was not converted back
properly to the smaller kinds and this ICEd (in certain cases).  The fix
is simply to add an appropriate conversion.  I do not entirely like the
way this is done in the patch (with the two variables and "special
casing"), but don't see a better implementation -- what do you think?

Regression-tested on x86_64-unknown-linux-gnu without failures -- though 
the run somehow looked strange to me (on the compile-farm); I'll try 
again to be sure.  Ok for trunk?

Yours,
Daniel

-- 
http://www.pro-vegan.info/
--
Done:  Arc-Bar-Cav-Kni-Ran-Rog-Sam-Tou-Val-Wiz
To go: Hea-Mon-Pri


[-- Attachment #2: patch.changelog --]
[-- Type: text/plain, Size: 271 bytes --]

2010-12-03  Daniel Kraft  <d@domob.eu>

	PR fortran/46794
	* trans-expr.c (gfc_conv_power_op): Handle kind of result expression
	correctly for integer kind 1 and 2 operands.

2010-12-03  Daniel Kraft  <d@domob.eu>

	PR fortran/46794
	* gfortran.dg/power2.f90: New test.


[-- Attachment #3: patch.diff --]
[-- Type: text/plain, Size: 2476 bytes --]

Index: gcc/testsuite/gfortran.dg/power2.f90
===================================================================
--- gcc/testsuite/gfortran.dg/power2.f90	(revision 0)
+++ gcc/testsuite/gfortran.dg/power2.f90	(revision 0)
@@ -0,0 +1,22 @@
+! { dg-do compile }
+! PR fortran/46794
+
+! Check that results of powers of integers with kinds 1 and 2 are
+! correctly converted back; this used to ICE because a conversion
+! from kind 4 to the correct one was missing.
+
+! Contributed by Daniel Kraft, d@domob.eu.
+
+PROGRAM main
+  IMPLICIT NONE
+
+  INTEGER(KIND=1) :: k1
+  INTEGER(KIND=2) :: k2
+
+  k1 = 1_1 + 1_1**k1
+  k2 = 1_2 + 1_2**k2
+
+  k2 = 1_1 + 1_1**k2
+  k2 = 1_1 + 1_2**k1
+  k2 = 1_1 + 1_2**k2
+END PROGRAM main
Index: gcc/fortran/trans-expr.c
===================================================================
--- gcc/fortran/trans-expr.c	(revision 167440)
+++ gcc/fortran/trans-expr.c	(working copy)
@@ -976,6 +976,7 @@
   tree gfc_int4_type_node;
   int kind;
   int ikind;
+  int res_ikind_1, res_ikind_2;
   gfc_se lse;
   gfc_se rse;
   tree fndecl = NULL;
@@ -996,6 +997,13 @@
 
   gfc_int4_type_node = gfc_get_int_type (4);
 
+  /* In case of integer operands with kinds 1 or 2, we call the integer kind 4
+     library routine.  But in the end, we have to convert the result back
+     if this case applies -- with res_ikind_K, we keep track whether operand K
+     falls into this case.  */
+  res_ikind_1 = -1;
+  res_ikind_2 = -1;
+
   kind = expr->value.op.op1->ts.kind;
   switch (expr->value.op.op2->ts.type)
     {
@@ -1006,6 +1014,7 @@
 	case 1:
 	case 2:
 	  rse.expr = convert (gfc_int4_type_node, rse.expr);
+	  res_ikind_2 = ikind;
 	  /* Fall through.  */
 
 	case 4:
@@ -1028,7 +1037,10 @@
 	case 1:
 	case 2:
 	  if (expr->value.op.op1->ts.type == BT_INTEGER)
-	    lse.expr = convert (gfc_int4_type_node, lse.expr);
+	    {
+	      lse.expr = convert (gfc_int4_type_node, lse.expr);
+	      res_ikind_1 = kind;
+	    }
 	  else
 	    gcc_unreachable ();
 	  /* Fall through.  */
@@ -1121,6 +1133,15 @@
 
   se->expr = build_call_expr_loc (input_location,
 			      fndecl, 2, lse.expr, rse.expr);
+
+  /* Convert the result back if it is of wrong integer kind.  */
+  if (res_ikind_1 != -1 && res_ikind_2 != -1)
+    {
+      /* We want the maximum of both operand kinds as result.  */
+      if (res_ikind_1 < res_ikind_2)
+	res_ikind_1 = res_ikind_2;
+      se->expr = convert (gfc_get_int_type (res_ikind_1), se->expr);
+    }
 }
 
 

Re: [Patch, Fortran] PR fortran/46794: Fix ICE with powers of integers

[Tobias Burnus]

Dear Daniel,

Daniel Kraft wrote:
> I do not entirely like the way this is done in the patch (with the two 
> variables and "special casing"), but don't see a better implementation 
> -- what do you think?

I think it is OK - at least I have trouble finding a better solution.

> Regression-tested on x86_64-unknown-linux-gnu without failures -- 
> though the run somehow looked strange to me (on the compile-farm); 
> I'll try again to be sure.  Ok for trunk?

OK for the trunk. Can you check whether one needs to likewise for the 
4.5 and 4.4 branch? (I think one should check on source level - the 
verify_tree might not always catch it. For some reasons, it ICEs here 
with 4.4 and 4.6 but not with 4.5; however, I think that's rather by 
chance and not because of a proper casting.)

Tobias

Re: [Patch, Fortran] PR fortran/46794: Fix ICE with powers of integers

[Daniel Kraft]

Hi Tobias,

Tobias Burnus wrote:
>> Regression-tested on x86_64-unknown-linux-gnu without failures -- 
>> though the run somehow looked strange to me (on the compile-farm); 
>> I'll try again to be sure.  Ok for trunk?
> 
> OK for the trunk. Can you check whether one needs to likewise for the 
> 4.5 and 4.4 branch? (I think one should check on source level - the 
> verify_tree might not always catch it. For some reasons, it ICEs here 
> with 4.4 and 4.6 but not with 4.5; however, I think that's rather by 
> chance and not because of a proper casting.)

No further problems with the regtest, thanks for the review!  I 
committed as rev. 167453 to trunk.  I will look at the source for 4.4 
and 4.5 accordingly.

Yours,
Daniel

-- 
http://www.pro-vegan.info/
--
Done:  Arc-Bar-Cav-Kni-Ran-Rog-Sam-Tou-Val-Wiz
To go: Hea-Mon-Pri

Re: [Patch, Fortran] PR fortran/46794: Fix ICE with powers of integers

[Daniel Kraft]

Tobias Burnus wrote:
>> Regression-tested on x86_64-unknown-linux-gnu without failures -- 
>> though the run somehow looked strange to me (on the compile-farm); 
>> I'll try again to be sure.  Ok for trunk?
> 
> OK for the trunk. Can you check whether one needs to likewise for the 
> 4.5 and 4.4 branch? (I think one should check on source level - the 
> verify_tree might not always catch it. For some reasons, it ICEs here 
> with 4.4 and 4.6 but not with 4.5; however, I think that's rather by 
> chance and not because of a proper casting.)

The code in question is (almost) identical for gcc 4.5 and gcc 4.4; the 
literal patch applied cleanly for 4.5 and regtested fine, backported as 
rev. 167454.  Now I'll look at 4.4.

Cheers,
Daniel


-- 
http://www.pro-vegan.info/
--
Done:  Arc-Bar-Cav-Kni-Ran-Rog-Sam-Tou-Val-Wiz
To go: Hea-Mon-Pri

Re: [Patch, Fortran] PR fortran/46794: Fix ICE with powers of integers

[Steve Kargl]

On Sat, Dec 04, 2010 at 10:35:16AM +0100, Daniel Kraft wrote:
> Hi Tobias,
> 
> Tobias Burnus wrote:
> >>Regression-tested on x86_64-unknown-linux-gnu without failures -- 
> >>though the run somehow looked strange to me (on the compile-farm); 
> >>I'll try again to be sure.  Ok for trunk?
> >
> >OK for the trunk. Can you check whether one needs to likewise for the 
> >4.5 and 4.4 branch? (I think one should check on source level - the 
> >verify_tree might not always catch it. For some reasons, it ICEs here 
> >with 4.4 and 4.6 but not with 4.5; however, I think that's rather by 
> >chance and not because of a proper casting.)
> 
> No further problems with the regtest, thanks for the review!  I 
> committed as rev. 167453 to trunk.  I will look at the source for 4.4 
> and 4.5 accordingly.
> 

Can you fix the test case to be valid Fortran.  k1 and k2
are used uninitialized.

-- 
Steve

Re: [Patch, Fortran] PR fortran/46794: Fix ICE with powers of integers

[Daniel Kraft]

[-- Attachment #1: Type: text/plain, Size: 1230 bytes --]

Steve Kargl wrote:
> On Sat, Dec 04, 2010 at 10:35:16AM +0100, Daniel Kraft wrote:
>> Hi Tobias,
>>
>> Tobias Burnus wrote:
>>>> Regression-tested on x86_64-unknown-linux-gnu without failures -- 
>>>> though the run somehow looked strange to me (on the compile-farm); 
>>>> I'll try again to be sure.  Ok for trunk?
>>> OK for the trunk. Can you check whether one needs to likewise for the 
>>> 4.5 and 4.4 branch? (I think one should check on source level - the 
>>> verify_tree might not always catch it. For some reasons, it ICEs here 
>>> with 4.4 and 4.6 but not with 4.5; however, I think that's rather by 
>>> chance and not because of a proper casting.)
>> No further problems with the regtest, thanks for the review!  I 
>> committed as rev. 167453 to trunk.  I will look at the source for 4.4 
>> and 4.5 accordingly.
>>
> 
> Can you fix the test case to be valid Fortran.  k1 and k2
> are used uninitialized.

This is only a compile-test.  But I committed the attached patch as 
obvious fix to trunk after a successful test on x86_64-unknown-linux-gnu 
and will consider this of course also for the backports.

Daniel

-- 
http://www.pro-vegan.info/
--
Done:  Arc-Bar-Cav-Kni-Ran-Rog-Sam-Tou-Val-Wiz
To go: Hea-Mon-Pri

[-- Attachment #2: patch.changelog --]
[-- Type: text/plain, Size: 107 bytes --]

2010-12-05  Daniel Kraft  <d@domob.eu>

	PR fortran/46794
	* gfortran.dg/power2.f90: Initialize variables.

[-- Attachment #3: patch.diff --]
[-- Type: text/plain, Size: 369 bytes --]

Index: gcc/testsuite/gfortran.dg/power2.f90
===================================================================
--- gcc/testsuite/gfortran.dg/power2.f90	(revision 167470)
+++ gcc/testsuite/gfortran.dg/power2.f90	(working copy)
@@ -13,6 +13,9 @@
   INTEGER(KIND=1) :: k1
   INTEGER(KIND=2) :: k2
 
+  k1 = 1_1
+  k2 = 1_2
+
   k1 = 1_1 + 1_1**k1
   k2 = 1_2 + 1_2**k2
 

Re: [Patch, Fortran] PR fortran/46794: Fix ICE with powers of integers

[Steve Kargl]

On Sun, Dec 05, 2010 at 11:18:14AM +0100, Daniel Kraft wrote:
> Steve Kargl wrote:
> >On Sat, Dec 04, 2010 at 10:35:16AM +0100, Daniel Kraft wrote:
> >>Hi Tobias,
> >>
> >>Tobias Burnus wrote:
> >>>>Regression-tested on x86_64-unknown-linux-gnu without failures -- 
> >>>>though the run somehow looked strange to me (on the compile-farm); 
> >>>>I'll try again to be sure.  Ok for trunk?
> >>>OK for the trunk. Can you check whether one needs to likewise for the 
> >>>4.5 and 4.4 branch? (I think one should check on source level - the 
> >>>verify_tree might not always catch it. For some reasons, it ICEs here 
> >>>with 4.4 and 4.6 but not with 4.5; however, I think that's rather by 
> >>>chance and not because of a proper casting.)
> >>No further problems with the regtest, thanks for the review!  I 
> >>committed as rev. 167453 to trunk.  I will look at the source for 4.4 
> >>and 4.5 accordingly.
> >>
> >
> >Can you fix the test case to be valid Fortran.  k1 and k2
> >are used uninitialized.
> 
> This is only a compile-test.  But I committed the attached patch as 
> obvious fix to trunk after a successful test on x86_64-unknown-linux-gnu 
> and will consider this of course also for the backports.
> 

Thanks!  I knew it was only a compile, but i would prefer valid
Fortran in the testsuite (when possible) because we do not know
if anyone else is try to use it as a validation suite.

-- 
Steve

Re: [Patch, Fortran] PR fortran/46794: Fix ICE with powers of integers

[Daniel Kraft]

[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]

Tobias Burnus wrote:
> Dear Daniel,
> 
> Daniel Kraft wrote:
>> I do not entirely like the way this is done in the patch (with the two 
>> variables and "special casing"), but don't see a better implementation 
>> -- what do you think?
> 
> I think it is OK - at least I have trouble finding a better solution.
> 
>> Regression-tested on x86_64-unknown-linux-gnu without failures -- 
>> though the run somehow looked strange to me (on the compile-farm); 
>> I'll try again to be sure.  Ok for trunk?
> 
> OK for the trunk. Can you check whether one needs to likewise for the 
> 4.5 and 4.4 branch? (I think one should check on source level - the 
> verify_tree might not always catch it. For some reasons, it ICEs here 
> with 4.4 and 4.6 but not with 4.5; however, I think that's rather by 
> chance and not because of a proper casting.)

Committed the attached (patch adapted to 4.4 including fix of test-case) 
as rev. 167644 to 4.4 branch.  When 4.5 is open again, I will fix the 
test-case there, too, and then close the PR.

Yours,
Daniel

-- 
http://www.pro-vegan.info/
--
Done:  Arc-Bar-Cav-Kni-Ran-Rog-Sam-Tou-Val-Wiz
To go: Hea-Mon-Pri

[-- Attachment #2: patch.changelog --]
[-- Type: text/plain, Size: 271 bytes --]

2010-12-09  Daniel Kraft  <d@domob.eu>

	PR fortran/46794
	* trans-expr.c (gfc_conv_power_op): Handle kind of result expression
	correctly for integer kind 1 and 2 operands.

2010-12-09  Daniel Kraft  <d@domob.eu>

	PR fortran/46794
	* gfortran.dg/power2.f90: New test.


[-- Attachment #3: patch.diff --]
[-- Type: text/plain, Size: 2467 bytes --]

Index: gcc/testsuite/gfortran.dg/power2.f90
===================================================================
--- gcc/testsuite/gfortran.dg/power2.f90	(revision 0)
+++ gcc/testsuite/gfortran.dg/power2.f90	(revision 0)
@@ -0,0 +1,25 @@
+! { dg-do compile }
+! PR fortran/46794
+
+! Check that results of powers of integers with kinds 1 and 2 are
+! correctly converted back; this used to ICE because a conversion
+! from kind 4 to the correct one was missing.
+
+! Contributed by Daniel Kraft, d@domob.eu.
+
+PROGRAM main
+  IMPLICIT NONE
+
+  INTEGER(KIND=1) :: k1
+  INTEGER(KIND=2) :: k2
+
+  k1 = 1_1
+  k2 = 1_2
+
+  k1 = 1_1 + 1_1**k1
+  k2 = 1_2 + 1_2**k2
+
+  k2 = 1_1 + 1_1**k2
+  k2 = 1_1 + 1_2**k1
+  k2 = 1_1 + 1_2**k2
+END PROGRAM main
Index: gcc/fortran/trans-expr.c
===================================================================
--- gcc/fortran/trans-expr.c	(revision 167632)
+++ gcc/fortran/trans-expr.c	(working copy)
@@ -917,6 +917,7 @@
   tree gfc_int4_type_node;
   int kind;
   int ikind;
+  int res_ikind_1, res_ikind_2;
   gfc_se lse;
   gfc_se rse;
   tree fndecl;
@@ -937,6 +938,13 @@
 
   gfc_int4_type_node = gfc_get_int_type (4);
 
+  /* In case of integer operands with kinds 1 or 2, we call the integer kind 4
+     library routine.  But in the end, we have to convert the result back
+     if this case applies -- with res_ikind_K, we keep track whether operand K
+     falls into this case.  */
+  res_ikind_1 = -1;
+  res_ikind_2 = -1;
+
   kind = expr->value.op.op1->ts.kind;
   switch (expr->value.op.op2->ts.type)
     {
@@ -947,6 +955,7 @@
 	case 1:
 	case 2:
 	  rse.expr = convert (gfc_int4_type_node, rse.expr);
+	  res_ikind_2 = ikind;
 	  /* Fall through.  */
 
 	case 4:
@@ -969,7 +978,10 @@
 	case 1:
 	case 2:
 	  if (expr->value.op.op1->ts.type == BT_INTEGER)
-	    lse.expr = convert (gfc_int4_type_node, lse.expr);
+	    {
+	      lse.expr = convert (gfc_int4_type_node, lse.expr);
+	      res_ikind_1 = kind;
+	    }
 	  else
 	    gcc_unreachable ();
 	  /* Fall through.  */
@@ -1080,6 +1092,15 @@
     }
 
   se->expr = build_call_expr (fndecl, 2, lse.expr, rse.expr);
+
+  /* Convert the result back if it is of wrong integer kind.  */
+  if (res_ikind_1 != -1 && res_ikind_2 != -1)
+    {
+      /* We want the maximum of both operand kinds as result.  */
+      if (res_ikind_1 < res_ikind_2)
+	res_ikind_1 = res_ikind_2;
+      se->expr = convert (gfc_get_int_type (res_ikind_1), se->expr);
+    }
 }
 
 

Re: [Patch, Fortran] PR fortran/46794: Fix ICE with powers of integers

[Daniel Kraft]

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]

Tobias Burnus wrote:
> Dear Daniel,
> 
> Daniel Kraft wrote:
>> I do not entirely like the way this is done in the patch (with the two 
>> variables and "special casing"), but don't see a better implementation 
>> -- what do you think?
> 
> I think it is OK - at least I have trouble finding a better solution.
> 
>> Regression-tested on x86_64-unknown-linux-gnu without failures -- 
>> though the run somehow looked strange to me (on the compile-farm); 
>> I'll try again to be sure.  Ok for trunk?
> 
> OK for the trunk. Can you check whether one needs to likewise for the 
> 4.5 and 4.4 branch? (I think one should check on source level - the 
> verify_tree might not always catch it. For some reasons, it ICEs here 
> with 4.4 and 4.6 but not with 4.5; however, I think that's rather by 
> chance and not because of a proper casting.)

Finally after 4.5 is open again, I committed the attached as test-case 
fix for 4.5 after a successful test on x86_64-unknown-linux-gnu. 
Closing the PR now.

Daniel

-- 
http://www.pro-vegan.info/
--
Done:  Arc-Bar-Cav-Kni-Ran-Rog-Sam-Tou-Val-Wiz
To go: Hea-Mon-Pri

[-- Attachment #2: patch.changelog --]
[-- Type: text/plain, Size: 107 bytes --]

2010-12-17  Daniel Kraft  <d@domob.eu>

	PR fortran/46794
	* gfortran.dg/power2.f90: Initialize variables.

[-- Attachment #3: patch.diff --]
[-- Type: text/plain, Size: 369 bytes --]

Index: gcc/testsuite/gfortran.dg/power2.f90
===================================================================
--- gcc/testsuite/gfortran.dg/power2.f90	(revision 167582)
+++ gcc/testsuite/gfortran.dg/power2.f90	(working copy)
@@ -13,6 +13,9 @@
   INTEGER(KIND=1) :: k1
   INTEGER(KIND=2) :: k2
 
+  k1 = 1_1
+  k2 = 1_2
+
   k1 = 1_1 + 1_1**k1
   k2 = 1_2 + 1_2**k2