[patch, fortran] Fix PR 69154, inline matmul with WHERE

[patch, fortran] Fix PR 69154, inline matmul with WHERE

[Thomas Koenig]

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

Hello world,

the attached patch fixes the regression.  Before this, front-end
optimization would have tried to put a DO loop inside a WHERE construct,
leading to an ICE.

Regression-tested. OK for trunk?

	Thomas

2016-01-10  Thomas Koenig  <tkoenig@gcc.gnu.org>

         PR fortran/69154
         * frontend-passes.c (in_where):  New variable.
         (inline_matmul_assign):  Don't try this if we are within
         a WHERE statement.
         (gfc_code_walker):  Keep track of in_where.

[-- Attachment #2: p1.diff --]
[-- Type: text/x-patch, Size: 1671 bytes --]

Index: frontend-passes.c
===================================================================
--- frontend-passes.c	(revision 232196)
+++ frontend-passes.c	(working copy)
@@ -78,6 +78,10 @@ static int forall_level;
 
 static bool in_omp_workshare;
 
+/* Keep track of whether we are within a WHERE statement.  */
+
+static bool in_where;
+
 /* Keep track of iterators for array constructors.  */
 
 static int iterator_level;
@@ -2790,6 +2794,9 @@ inline_matmul_assign (gfc_code **c, int *walk_subt
   if (co->op != EXEC_ASSIGN)
     return 0;
 
+  if (in_where)
+    return 0;
+
   expr1 = co->expr1;
   expr2 = co->expr2;
   if (expr2->expr_type != EXPR_FUNCTION
@@ -3268,6 +3275,7 @@ gfc_code_walker (gfc_code **c, walk_code_fn_t code
 	  gfc_code *co;
 	  gfc_association_list *alist;
 	  bool saved_in_omp_workshare;
+	  bool saved_in_where;
 
 	  /* There might be statement insertions before the current code,
 	     which must not affect the expression walker.  */
@@ -3274,6 +3282,7 @@ gfc_code_walker (gfc_code **c, walk_code_fn_t code
 
 	  co = *c;
 	  saved_in_omp_workshare = in_omp_workshare;
+	  saved_in_where = in_where;
 
 	  switch (co->op)
 	    {
@@ -3301,6 +3310,10 @@ gfc_code_walker (gfc_code **c, walk_code_fn_t code
 	      WALK_SUBEXPR (co->ext.iterator->step);
 	      break;
 
+	    case EXEC_WHERE:
+	      in_where = true;
+	      break;
+
 	    case EXEC_CALL:
 	    case EXEC_ASSIGN_CALL:
 	      for (a = co->ext.actual; a; a = a->next)
@@ -3554,6 +3567,7 @@ gfc_code_walker (gfc_code **c, walk_code_fn_t code
 	    doloop_level --;
 
 	  in_omp_workshare = saved_in_omp_workshare;
+	  in_where = saved_in_where;
 	}
     }
   return 0;

[-- Attachment #3: inline_matmul_12.f90 --]
[-- Type: text/x-fortran, Size: 492 bytes --]

! { dg-do compile }
! PR 69154 - inlined matmul used to cause an ICE inside a WHERE.
MODULE m_numeric_tools
 INTEGER, PARAMETER :: dp=8
CONTAINS
subroutine llsfit_svd(xx,yy,sigma,nfuncs,funcs,chisq,par,var,cov,info)
 real(dp),intent(in) :: xx(:),yy(:),sigma(:)
 real(dp),dimension(SIZE(xx)) :: bb,sigm1
 real(dp) :: tmp(nfuncs)
 real(dp),allocatable :: work(:),Vt(:,:),U(:,:),S(:)
 WHERE (S>TOL_*MAXVAL(S))
  tmp=MATMUL(bb,U)/S
 END WHERE
end subroutine llsfit_svd
END MODULE m_numeric_tools

Re: [patch, fortran] Fix PR 69154, inline matmul with WHERE

[Thomas Koenig]

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

Am 10.01.2016 um 12:55 schrieb Thomas Koenig:
> Hello world,
>
> the attached patch fixes the regression.  Before this, front-end
> optimization would have tried to put a DO loop inside a WHERE construct,
> leading to an ICE.

The updated test case demonstrates that matmul inlining is done even
after not doing it inside a WHERE.

OK for trunk with that change?

	Thomas


[-- Attachment #2: inline_matmul_12.f90 --]
[-- Type: text/x-fortran, Size: 717 bytes --]

! { dg-do compile }
! { dg-options "-ffrontend-optimize -fdump-tree-original" }
! PR 69154 - inlined matmul used to cause an ICE inside a WHERE.
MODULE m_numeric_tools
 INTEGER, PARAMETER :: dp=8
CONTAINS
subroutine llsfit_svd(xx,yy,sigma,nfuncs,funcs,chisq,par,var,cov,info)
 real(dp),intent(in) :: xx(:),yy(:),sigma(:)
 real(dp),dimension(SIZE(xx)) :: bb,sigm1
 real(dp) :: tmp(nfuncs)
 real(dp),allocatable :: work(:),Vt(:,:),U(:,:),S(:)
 real(dp),dimension(3,3) :: a, b, c
 WHERE (S>TOL_*MAXVAL(S))
  tmp=MATMUL(bb,U)/S
 END WHERE
 call random_number(a)
 call random_number(b)
 c = matmul(a,b)
end subroutine llsfit_svd

END MODULE m_numeric_tools
! { dg-final { scan-tree-dump-times "matmul_r8" 1 "original" } }

Re: [patch, fortran] Fix PR 69154, inline matmul with WHERE

[Toon Moene]

On 01/10/2016 12:55 PM, Thomas Koenig wrote:

> Hello world,
>
> the attached patch fixes the regression.  Before this, front-end
> optimization would have tried to put a DO loop inside a WHERE construct,
> leading to an ICE.

Thomas,

Am I correct in assuming that your inlining of MATMUL only treats the 
form MATMUL(A, B) and not MATMUL(A, TRANSPOSE(B)) ?

Our code has about half of one and half of the other ...

Thanks for your answer,

-- 
Toon Moene - e-mail: toon@moene.org - phone: +31 346 214290
Saturnushof 14, 3738 XG  Maartensdijk, The Netherlands
At home: http://moene.org/~toon/; weather: http://moene.org/~hirlam/
Progress of GNU Fortran: http://gcc.gnu.org/wiki/GFortran#news

Re: [patch, fortran] Fix PR 69154, inline matmul with WHERE

[Steve Kargl]

On Mon, Jan 11, 2016 at 10:58:26PM +0100, Toon Moene wrote:
> 
> Am I correct in assuming that your inlining of MATMUL only treats the 
> form MATMUL(A, B) and not MATMUL(A, TRANSPOSE(B)) ?
> 

program u8
   real a(3,3), b(3,3), c(3,3)
   call random_number(a)
   call random_number(b)
   c = matmul(a,transpose(b))
   print *,c(1,1)
end program u8

% gfc6 -O2 -o z -fdump-tree-original u8.f90 && grep matmul u8.f90.003t.original 
    _gfortran_matmul_r4 (&parm.2, D.3427, D.3429, 0, 0, 0B);

-- 
Steve

Re: [patch, fortran] Fix PR 69154, inline matmul with WHERE

[Thomas Koenig]

Hi Toon,

> Am I correct in assuming that your inlining of MATMUL only treats the
> form MATMUL(A, B) and not MATMUL(A, TRANSPOSE(B)) ?
>
> Our code has about half of one and half of the other ...

This is, currently, correct, as Steve pointed out.

I have been doing some work on extending this to TRANSPOSE, but have
gotten sidetracked by other things in real life...

Basically, it would mean extending check_conjg_variable to look for
uneven numbers of TRANSPOSE as well, and then change the DO loops
and scalarization accordingly.

This is the subject of PR 66904.

Too late for 6.1, unfortunatly.

Regards

	Thomas

Re: [patch, fortran] Fix PR 69154, inline matmul with WHERE

[Toon Moene]

On 01/12/2016 12:01 AM, Thomas Koenig wrote:

> Hi Toon,
>
>> Am I correct in assuming that your inlining of MATMUL only treats the
>> form MATMUL(A, B) and not MATMUL(A, TRANSPOSE(B)) ?
>>
>> Our code has about half of one and half of the other ...
>
> This is, currently, correct, as Steve pointed out.

Yep, I see.

Still, one example of our MATMUL(A, TRANSPOSE(B)) occurrences has B 
being a 65*65 square matrix, with A being (roughly) 22,000 * 65.

I surely going to see what happens, performance wise, if I just change 
the expression to the following lines (TMP being a 65*65 local array):

TMP = TRANSPOSE(B)
... MATMUL(A, TMP)

Kind regards and thanks for your work on this,

-- 
Toon Moene - e-mail: toon@moene.org - phone: +31 346 214290
Saturnushof 14, 3738 XG  Maartensdijk, The Netherlands
At home: http://moene.org/~toon/; weather: http://moene.org/~hirlam/
Progress of GNU Fortran: http://gcc.gnu.org/wiki/GFortran#news

Re: [patch, fortran] Fix PR 69154, inline matmul with WHERE

[Thomas Koenig]

Am 12.01.2016 um 00:17 schrieb Toon Moene:
> On 01/12/2016 12:01 AM, Thomas Koenig wrote:
>

>>> Am I correct in assuming that your inlining of MATMUL only treats the
>>> form MATMUL(A, B) and not MATMUL(A, TRANSPOSE(B)) ?
>>>
>>> Our code has about half of one and half of the other ...
>>
>> This is, currently, correct, as Steve pointed out.
>
> Yep, I see.
>
> Still, one example of our MATMUL(A, TRANSPOSE(B)) occurrences has B
> being a 65*65 square matrix, with A being (roughly) 22,000 * 65.
>
> I surely going to see what happens, performance wise, if I just change
> the expression to the following lines (TMP being a 65*65 local array):
>
> TMP = TRANSPOSE(B)
> ... MATMUL(A, TMP)
>

If you are going to do some tests, you could also test (assuming
you're on a 64-bit system)


   integer(kind=8) :: i, j, k

   do i=0, size(b,1)-1
      do k=0, size(a,2)-1
         do j=0, size(a,1)-1
            c(j+1,i+1) = c(j+1,i+1) + a(j+1,k+1) * b(i+1, k+1)
         end do
      end do
   end do

because this is what the unrolling will produce for
c = matmul(a,transpose(b)).

Regards

	Thomas

Re: [patch, fortran] Fix PR 69154, inline matmul with WHERE

[Toon Moene]

On 01/12/2016 08:33 AM, Thomas Koenig wrote:
> Am 12.01.2016 um 00:17 schrieb Toon Moene:
>> On 01/12/2016 12:01 AM, Thomas Koenig wrote:
>>
>
>>>> Am I correct in assuming that your inlining of MATMUL only treats the
>>>> form MATMUL(A, B) and not MATMUL(A, TRANSPOSE(B)) ?
>>>>
>>>> Our code has about half of one and half of the other ...
>>>
>>> This is, currently, correct, as Steve pointed out.
>>
>> Yep, I see.
>>
>> Still, one example of our MATMUL(A, TRANSPOSE(B)) occurrences has B
>> being a 65*65 square matrix, with A being (roughly) 22,000 * 65.
>>
>> I surely going to see what happens, performance wise, if I just change
>> the expression to the following lines (TMP being a 65*65 local array):
>>
>> TMP = TRANSPOSE(B)
>> ... MATMUL(A, TMP)
>>
>
> If you are going to do some tests, you could also test (assuming
> you're on a 64-bit system)
>

Good idea.

I suppose I have to add a line:
>
>    integer(kind=8) :: i, j, k
>
>    do i=0, size(b,1)-1

         c(:,i+1) = 0.0

?
>       do k=0, size(a,2)-1
>          do j=0, size(a,1)-1
>             c(j+1,i+1) = c(j+1,i+1) + a(j+1,k+1) * b(i+1, k+1)
>          end do
>       end do
>    end do
>
> because this is what the unrolling will produce for
> c = matmul(a,transpose(b)).
>
> Regards
>
>      Thomas
>


-- 
Toon Moene - e-mail: toon@moene.org - phone: +31 346 214290
Saturnushof 14, 3738 XG  Maartensdijk, The Netherlands
At home: http://moene.org/~toon/; weather: http://moene.org/~hirlam/
Progress of GNU Fortran: http://gcc.gnu.org/wiki/GFortran#news

Re: [patch, fortran] Fix PR 69154, inline matmul with WHERE

[Paul Richard Thomas]

Hi Thomas,

Thanks for changing the testcase to demonstrate that in_where does not
get left as true.

OK for trunk.

That's one regression down!

Thanks for the patch

Paul

On 10 January 2016 at 13:38, Thomas Koenig <tkoenig@netcologne.de> wrote:
> Am 10.01.2016 um 12:55 schrieb Thomas Koenig:
>>
>> Hello world,
>>
>> the attached patch fixes the regression.  Before this, front-end
>> optimization would have tried to put a DO loop inside a WHERE construct,
>> leading to an ICE.
>
>
> The updated test case demonstrates that matmul inlining is done even
> after not doing it inside a WHERE.
>
> OK for trunk with that change?
>
>         Thomas
>



-- 
The difference between genius and stupidity is; genius has its limits.

Albert Einstein