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
