| Message ID | 0393d2b1-849e-e4d4-b505-7f6ed3d52c89@netcologne.de |
|---|---|
| State | New |
| Headers | show |
| Series | [fortran] Introduce -finline-pack | expand |
On Sat, Dec 7, 2019 at 2:53 PM Thomas Koenig <tkoenig@netcologne.de> wrote: > > Hello world, > > the attached patch introduces a new option, -finline-pack. > > Since the fix for PR88821, we now do inline packing of > arguments (if required) via the scalarizer, instead of > using _gfortran_internal_[un]pack when optimizing, but > not when optimizing for size. > > This introduces (really) large performance gains for some test > cases because now the middle end can see through the packing. > On the other hand, for test cases which do a _lot_ of this, > compile time and code size can increase by quite a bit. > > So, this patch introduces an option to control that behavior, > so that people can turn it off on a by-file basis if they > don't want it. > > OK for trunk? Just as a suggestion, maybe we'd want to extend this to other intrinsics in future so a -fno-inline-intrinsic=pack[,...] is more future proof? (I'd inline all intrinsics by default thus only provide the negative form). You can avoid the extra option parsing complexity by only literally adding -fno-inline-intrinsic=pack for now. Richard. > Regards > > Thomas > > Introduce -finline-pack. > > 2019-12-07 Thomas Koenig <tkoenig@gcc.gnu.org> > > PR middle-end/91512 > PR fortran/92738 > * invoke.texi: Document -finline-pack. > * lang.opt: Add -finline-pack. > * options.c (gfc_post_options): Handle -finline-pack. > * trans-array.c (gfc_conv_array_parameter): Use flag_inline_pack > instead of checking for optimize and optimize_size. > > 2019-12-07 Thomas Koenig <tkoenig@gcc.gnu.org> > > PR middle-end/91512 > PR fortran/92738 > * gfortran.dg/inline_pack_25.f90: New test.
Hi Richard, > Just as a suggestion, maybe we'd want to extend this > to other intrinsics in future so a -fno-inline-intrinsic=pack[,...] > is more future proof? (I'd inline all intrinsics by default thus > only provide the negative form). You can avoid the extra > option parsing complexity by only literally adding > -fno-inline-intrinsic=pack for now. I agree that such an option would make sense, I think this is something we should consider for gcc 11. In this instance, your reply shows that the option is poorly named, because it is actually not about the PACK intrinsic, but the internal packing that happens for arguments. Maybe -finline-repack would be a better name? -finline-internal-pack? Regards Thomas
Am 09.12.19 um 17:30 schrieb Thomas Koenig:
> Maybe -finline-repack would be a better name? -finline-internal-pack?
Steve made an excellent suggestion: -finline-arg-packing .
So, OK with that change?
Regards
Thomas
Am 10.12.19 um 22:22 schrieb Thomas Koenig: > Steve made an excellent suggestion: -finline-arg-packing . > > So, OK with that change? In other words, is https://gcc.gnu.org/ml/gcc-patches/2019-12/msg00485.html OK with renaming the option to -finline-arg-packing ? Regards Thomas
On Sun, Dec 15, 2019 at 07:11:25PM +0100, Thomas Koenig wrote: > Am 10.12.19 um 22:22 schrieb Thomas Koenig: > > Steve made an excellent suggestion: -finline-arg-packing . > > > > So, OK with that change? > > In other words, is > > https://gcc.gnu.org/ml/gcc-patches/2019-12/msg00485.html > > OK with renaming the option to -finline-arg-packing ? > I think it's ok.
On Sun, Dec 15, 2019 at 07:11:25PM +0100, Thomas Koenig wrote: > Am 10.12.19 um 22:22 schrieb Thomas Koenig: > > Steve made an excellent suggestion: -finline-arg-packing . > > > > So, OK with that change? > > In other words, is > > https://gcc.gnu.org/ml/gcc-patches/2019-12/msg00485.html > > OK with renaming the option to -finline-arg-packing ? This patch broke: +FAIL: compiler driver --help=fortran option(s): "^ +-.*[^:.]\$" absent from output: " -finline-arg-packing Perform argument packing inline" That test verifies that the help texts of all options are terminated with dot or semicolon. Fixed thusly, tested with make check-gcc RUNTESTFLAGS=help.exp. I've additionally noticed you've swapped the two ChangeLog entries, testsuite/ one went into fortran/ and vice versa, swapped that too (and that is the reason why I'm posting exactly what I've committed as obvious, rather than ChangeLog entry before patch + patch). --- fortran/lang.opt (revision 279686) +++ fortran/lang.opt (revision 279687) @@ -649,7 +649,7 @@ Enum(gfc_init_local_real) String(-inf) V finline-arg-packing Fortran Var(flag_inline_arg_packing) Init(-1) --finline-arg-packing Perform argument packing inline +-finline-arg-packing Perform argument packing inline. finline-matmul-limit= Fortran RejectNegative Joined UInteger Var(flag_inline_matmul_limit) Init(-1) --- testsuite/ChangeLog (revision 279686) +++ testsuite/ChangeLog (revision 279687) @@ -51,12 +51,7 @@ PR middle-end/91512 PR fortran/92738 - * invoke.texi: Document -finline-arg-packing. - * lang.opt: Add -finline-arg-packing. - * options.c (gfc_post_options): Handle -finline-arg-packing. - * trans-array.c (gfc_conv_array_parameter): Use - flag_inline_arg_packing instead of checking for optimize and - optimize_size. + * gfortran.dg/inline_pack_25.f90: New test. 2019-12-20 Tobias Burnus <tobias@codesourcery.com> --- fortran/ChangeLog (revision 279686) +++ fortran/ChangeLog (revision 279687) @@ -1,8 +1,19 @@ +2019-12-20 Jakub Jelinek <jakub@redhat.com> + + PR middle-end/91512 + PR fortran/92738 + * lang.opt (-finline-arg-packing): Add trailing dot to help text. + 2019-12-20 Thomas Koenig <tkoenig@gcc.gnu.org> PR middle-end/91512 PR fortran/92738 - * gfortran.dg/inline_pack_25.f90: New test. + * invoke.texi: Document -finline-arg-packing. + * lang.opt: Add -finline-arg-packing. + * options.c (gfc_post_options): Handle -finline-arg-packing. + * trans-array.c (gfc_conv_array_parameter): Use + flag_inline_arg_packing instead of checking for optimize and + optimize_size. 2019-12-20 Tobias Burnus <tobias@codesourcery.com> Jakub
Hi Jakub, > This patch broke: > +FAIL: compiler driver --help=fortran option(s): "^ +-.*[^:.]\$" absent from output: " -finline-arg-packing Perform argument packing inline" > That test verifies that the help texts of all options are terminated > with dot or semicolon. Thanks, and sorry for the breakage. Note to self: Try not to forget that dot. This was not caught with "make check-fortran" from the gcc build directory. Maybe it would be a good idea to add that test to check-fortran too. Does anybody have an idea how to do that? Regards Thomas
Index: invoke.texi =================================================================== --- invoke.texi (Revision 279064) +++ invoke.texi (Arbeitskopie) @@ -192,8 +192,9 @@ and warnings}. -ffrontend-loop-interchange -ffrontend-optimize @gol -finit-character=@var{n} -finit-integer=@var{n} -finit-local-zero @gol -finit-derived -finit-logical=@var{<true|false>} @gol --finit-real=@var{<zero|inf|-inf|nan|snan>} @gol --finline-matmul-limit=@var{n} -fmax-array-constructor=@var{n} @gol +-finit-real=@var{<zero|inf|-inf|nan|snan>} +-finline-matmul-limit=@var{n} @gol +-finline-pack -fmax-array-constructor=@var{n} @gol -fmax-stack-var-size=@var{n} -fno-align-commons -fno-automatic @gol -fno-protect-parens -fno-underscoring -fsecond-underscore @gol -fpack-derived -frealloc-lhs -frecursive -frepack-arrays @gol @@ -1779,6 +1780,34 @@ compiled with the @option{-fshort-enums} option. GNU Fortran choose the smallest @code{INTEGER} kind a given enumerator set will fit in, and give all its enumerators this kind. +@item -finline-pack +@opindex @code{finline-pack} +When passing an assumed-shape argument of a procedure as actual +argument to an assumed-size or explicit size or as argument to a +procedure that does not have an explicit interface, the argument may +have to be packed, that is put into contiguous memory. An example is +the call to @code{foo} in +@smallexample + subroutine foo(a) + real, dimension(*) :: a + end subroutine foo + subroutine bar(b) + real, dimension(:) :: b + call foo(b) + end subroutine bar +@end smallexample + +When @option{-finline-pack} is in effect, this packing will be +performed by inline code. This allows for more optimization while +increasing code size. + +@option{-finlie-pack} is implied by any of the @option{-O} options +except when optimizing for size via @option{-Os}. If the code +contains a very large number of argument that have to be packed, code +size and also compilation time may become excessive. If that is the +case, it may be better to disable this option. Instances of packing +can be found by using by using @option{-Warray-temporaries}. + @item -fexternal-blas @opindex @code{fexternal-blas} This option will make @command{gfortran} generate calls to BLAS functions Index: lang.opt =================================================================== --- lang.opt (Revision 279064) +++ lang.opt (Arbeitskopie) @@ -647,6 +647,10 @@ Enum(gfc_init_local_real) String(inf) Value(GFC_IN EnumValue Enum(gfc_init_local_real) String(-inf) Value(GFC_INIT_REAL_NEG_INF) +finline-pack +Fortran Var(flag_inline_pack) Init(-1) +-finline-pack Perform argument packing inline + finline-matmul-limit= Fortran RejectNegative Joined UInteger Var(flag_inline_matmul_limit) Init(-1) -finline-matmul-limit=<n> Specify the size of the largest matrix for which matmul will be inlined. Index: options.c =================================================================== --- options.c (Revision 279064) +++ options.c (Arbeitskopie) @@ -467,6 +467,11 @@ gfc_post_options (const char **pfilename) if (flag_frontend_loop_interchange == -1) flag_frontend_loop_interchange = optimize; + /* Do inline packing by default if optimizing, but not if + optimizing for size. */ + if (flag_inline_pack == -1) + flag_inline_pack = optimize && !optimize_size; + if (flag_max_array_constructor < 65535) flag_max_array_constructor = 65535; Index: trans-array.c =================================================================== --- trans-array.c (Revision 279064) +++ trans-array.c (Arbeitskopie) @@ -8139,7 +8139,7 @@ gfc_conv_array_parameter (gfc_se * se, gfc_expr * making the packing and unpacking operation visible to the optimizers. */ - if (g77 && optimize && !optimize_size && expr->expr_type == EXPR_VARIABLE + if (g77 && flag_inline_pack && expr->expr_type == EXPR_VARIABLE && !is_pointer (expr) && ! gfc_has_dimen_vector_ref (expr) && (fsym == NULL || fsym->ts.type != BT_ASSUMED)) {