Re: [Patch, fortran] PR 37131, inline matmul

[Mikael Morin <mikael dot morin at sfr dot fr>]

Le 05/05/2015 08:25, Thomas Koenig a Ãcrit :
> Hello world,
> 
> this is an update of the matmul inline patch.  The only difference to
> the last version is that it has the ubound simplification taken out.

Sorry, I delayed this, but it wasn't (yet) forgotten.

Below a few answers to
https://gcc.gnu.org/ml/gcc-patches/2015-04/msg01247.html
and documentation fixes.


> 	* simplify.c (simplify_bound):  Simplify the case of the
> 	lower bound of an assumed-shape argument.
Entry to be removed. ;-)


>>> Index: fortran/array.c
>>> ===================================================================
>>> --- fortran/array.c	(Revision 222218)
>>> +++ fortran/array.c	(Arbeitskopie)
>>> @@ -338,6 +338,9 @@ gfc_resolve_array_spec (gfc_array_spec *as, int ch
>>>    if (as == NULL)
>>>      return true;
>>>  
>>> +  if (as->resolved)
>>> +    return true;
>>> +
>> Why this?
> 
> Because you get regressions otherwise.  Not resolving an array spec
> twice should do no harm, and resolving it twice does so - I hit the
> error message in check_restricted.  I'm not sure what is wrong, maybe
> PR 23466 was not fully fixed, but this works.
> 
Hum, it seems to work without it here.  Can you double check?

>>> @@ -524,29 +542,11 @@ constant_string_length (gfc_expr *e)
>>>  
>>>  }
>>>  
>>> -/* Returns a new expression (a variable) to be used in place of the old one,
>>> -   with an assignment statement before the current statement to set
>>> -   the value of the variable. Creates a new BLOCK for the statement if
>>> -   that hasn't already been done and puts the statement, plus the
>>> -   newly created variables, in that block.  Special cases:  If the
>>> -   expression is constant or a temporary which has already
>>> -   been created, just copy it.  */
>>> -
>>> -static gfc_expr*
>>> -create_var (gfc_expr * e)
>> Keep a comment here.
> 
> Still exists, further down.
> 
I don't mind the comment being moved around together with create_var. :-)
I was asking for a comment for the new function insert_block.


> 
>>> +	      gfc_simplify_expr (ar->start[i], 0);
>>> +	    }
>>> +	  else if (was_fullref)
>>> +	    {
>>> +	      ar->dimen_type[i] = DIMEN_RANGE;
>>> +	      ar->start[i] = NULL;
>>> +	      ar->end[i] = NULL;
>>> +	      ar->stride[i] = NULL;
>>> +	    }
>> Is this reachable ?
> 
> Not in the current incarnation, I wanted to keep it around for
> a full segment later.  I can also remove this.
> 
At least add an assert, a comment, something telling that it's not used.
I would rather remove it until it's actually used, but I can live with
it, if it becomes used shortly.

>>
>> [...]
>>
>>> Index: fortran/options.c
>>> ===================================================================
>>> --- fortran/options.c	(Revision 222218)
>>> +++ fortran/options.c	(Arbeitskopie)
[...]
>>> +
>>> +  if (flag_external_blas && flag_inline_matmul_limit < 0)
>>> +    flag_inline_matmul_limit = flag_blas_matmul_limit;
>> Hum, shouldn't we do something for flag_inline_matmul_limit > 0 as well?
> 
> 
> This is done automatically, by the options machinery.  That is cool 
> 
Huh? is it?
I was talking about this:
Using -fblas-matmul-limit=10, one gets inlining until 10.
Using -fblas-matmul-limit=10 -finline-matmul-limit=20 the inlining limit
is _increased_ to 20.

It is of course questionable for a user to specify a low limit for blas
being lower than the high limit for inlining, so that we have a
contradictory specification for the matrix sizes in between.
But I think it would make more sense to have blas take precedence, that
is have flag_inline_matmul_limit clamped to flag_blas_matmul_limit, even
for flag_inline_matmul_limit > 0.
Is this done by the options machinery?

Either way, I don't mind too much, this is a corner case.


> Index: invoke.texi
> ===================================================================
> --- invoke.texi	(Revision 222218)
> +++ invoke.texi	(Arbeitskopie)
> @@ -1537,6 +1538,20 @@ geometric mean of the dimensions of the argument a
>  
>  The default value for @var{n} is 30.
>  
> +@item -finline-matmul-limit=@var{n}
> +@opindex @code{finline-matmul-limit}
> +When front-end optimiztion is active,
optimization
> some calls to the @code{MATMUL}
> +intrinsic function will be inlined.
s/some calls ... inlined/calls ... inlined for small matrix sizes/
or something like that.

>  Setting
> +@code{-finline-matmul-limit=0} will disable inlining in all cases.

> +Setting this option it to a specified value will call the library
> +routines for matrices with size larger than @var{n}. 
s/it //
Setting ... value @var{n} will produce inline code for matrix sizes up
to @var{n}; the library routines will be called for bigger matrices.

Maybe add something about code bloat here.  Suggestion:

This may result in code size increase if the matrix size can't be
determined at compile time, as code for both cases is generated.

> If the matrices
> +involved are not square, the size comparison is performed using the
> +geometric mean of the dimensions of the argument and result matrices.
> +
> +The default value for @var{n} is the value specified for
> +@code{-fblas-matmul-limit} if this option is specified, or unlimitited
unlimitted
> +otherwise.
> +
>  @item -frecursive
>  @opindex @code{frecursive}
>  Allow indirect recursion by forcing all local arrays to be allocated



With the resolved thing removed, the comment before insert_block and the
doc fixes, the patch is OK.
The rest is up to you.  Thanks.

Mikael